feat: add foreground color style on iOS #266
Conversation
IvanIhnatsiuk
changed the title
|
Hey @IvanIhnatsiuk, it's super nice to get such verbose contribution from the community! While the feature is potentially interesting for us we cannot really proceed with it for now; we want the API and new features to be working on both mobile platforms from the get-go. Either way, great work! |
IvanIhnatsiuk
force-pushed
the
feat/foreground-color-style-for-iOS
branch
2 times, most recently
from
2605503 to
e3133c6
Compare
IvanIhnatsiuk
force-pushed
the
feat/foreground-color-style-for-iOS
branch
from
910ce22 to
cae71fd
Compare
exploIF
left a comment
exploIF
left a comment
There was a problem hiding this comment.
I've added couple general comments. iOS part is still not reviewed.
Our main goal currently is to have consistent behavior and functionality between platforms. So I'm afraid we won't be able to ship that until Android part is implemented.
However, if this feature is something that community would like to see, we may consider implementing Android part ourselves
There was a problem hiding this comment.
Those changes seems completely unrelated to PR scope
There was a problem hiding this comment.
Same here, let's keep this comment
| color: string; | ||
| }; | ||
|
|
||
| const ColorPreview: React.FC<Props> = ({ color }) => { |
There was a problem hiding this comment.
As a convention, we do not rely on global variables. so FC should be imported from React
| }, | ||
| }); | ||
|
|
||
| export default ColorPreview; |
There was a problem hiding this comment.
As a convention, we use named exports
| const containerStyle = useMemo( | ||
| () => [ | ||
| styles.container, | ||
| { backgroundColor: isActive ? color : 'rgba(0, 26, 114, 0.8)' }, | ||
| ], | ||
| [isActive, color] | ||
| ); |
There was a problem hiding this comment.
useMemo seems redundant here, it's simple computation
There was a problem hiding this comment.
- I added this because of the warning about embedded styles.
- useMemo is useful here because it is an array that will create a new reference each time it is re-rendered and cause re-rendering in child components. Ideally, all values that arecallbacks, arrays and objects should be wrapped with useCallback/useMemo if you are not using react-compiler (sometimes even if you are using it).
| text: string, | ||
| payload: string | ||
| ) => void; | ||
| toggleColor: ( |
There was a problem hiding this comment.
I think it should be rather called setColor, as we are explicitly passing color that should be used. We use toggle for two state styles (which is either enabled or disabled)
IvanIhnatsiuk
force-pushed
the
feat/foreground-color-style-for-iOS
branch
from
cae71fd to
910ce22
Compare
IvanIhnatsiuk
force-pushed
the
feat/foreground-color-style-for-iOS
branch
from
83c7f25 to
7e1cbde
Compare
3 participants
Summary
In this PR, I added foreground color style support for iOS. Currently, it allows changing color in any kind of style, and it uses the font tag to identify colored parts of attributed text from HTML. To track color in already colored styles, we need to check that the current location is in this style and the color is not the same as the configured one.
Also, I'm not sure about the onSelectionColor event, do we need to run it only when we're in the color style, or would it be better to do it always?
Test Plan
Screenshots / Videos
Screen.Recording.2025-11-19.at.23.18.54.mov
Include any visual proof that helps reviewers understand the change — UI updates, bug reproduction or the result of the fix.
Compatibility