refactor: replace string with ColorValue for ThemeColors#4945
Conversation
|
Hey @adrcotfas, thank you for your pull request 🤗. The documentation from this branch can be viewed here. |
|
The mobile version of example app from this branch is ready! You can see it here. |
adrcotfas
force-pushed
the
@adrcotfas/refactor/dynamic_themes
branch
from
91d863b to
c8c60d0
Compare
adrcotfas
force-pushed
the
@adrcotfas/refactor/color_value
branch
from
2218eb6 to
cb8fee4
Compare
| descriptors[route.key].options.tabBarIcon?.({ | ||
| focused, | ||
| color, | ||
| color: typeof color === 'string' ? color : '', |
There was a problem hiding this comment.
updated react navigation to latest alpha so ColorValue will work #4948
ps: in future throw an error for such scenarios instead of using empty string, so any problems due to future changes are not suppressed
There was a problem hiding this comment.
I see you modified this part in that PR.
I assume I can merge this first since you approved it.
| if (isSelectedColor) { | ||
| return color(selectedColor).alpha(0.29).rgb().string(); | ||
| if (typeof selectedColor === 'string') { | ||
| return color(selectedColor).alpha(0.29).rgb().string(); |
There was a problem hiding this comment.
we'll need to tweak how we handle color operations so we also support things like var( (for web), which are also strings. could create a utility function that handles these properly.
There was a problem hiding this comment.
This part is legacy. We'll anyway modernize each component and this old way of applying alpha will be replaced by using theme color tokens.
I'm not sure about web but I assume that we'll pass the colors in the theme and not in components.
Since you approved this PR I assume you're not requesting an utility function here and now.
adrcotfas
force-pushed
the
@adrcotfas/refactor/dynamic_themes
branch
from
c8c60d0 to
37e7ed6
Compare
adrcotfas
force-pushed
the
@adrcotfas/refactor/color_value
branch
from
cb8fee4 to
7380d04
Compare
adrcotfas
force-pushed
the
@adrcotfas/refactor/color_value
branch
from
7380d04 to
6b7ce40
Compare
|
The mobile version of example app from this branch is ready! You can see it here. |
3 participants
Motivation
Mentioned here: #4925 (comment)
Related issue
#4925
Test plan
yarn typescript-- no new type errorsyarn test-- all tests pass