-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RangeControl: Adapt slider color to color scheme #23936
Conversation
Size Change: +33 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
@@ -137,9 +137,9 @@ export const ALERT = { | |||
export const UI = { | |||
background: BASE.white, | |||
backgroundDisabled: LIGHT_GRAY[ 200 ], | |||
brand: BLUE.wordpress[ 700 ], | |||
brand: `var( --wp-admin-theme-color, ${ BLUE.wordpress[ 700 ] })`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should actually keep then aming adminThemeColor
instead of brand
and adminThemeColorDarker10
for the "borderFocus" one? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think swapping brand
to adminThemeColor
is okay. Or maybe just themeColor
👍
However, I think keeping the other names like border
or borderFocus
is important, as it can be understood without having prior knowledge (in this case, knowledge of the naming and availability of the --wp-admin-theme
color variant)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I think keeping the other names like border or borderFocus is important, as it can be understood without having prior knowledge (in this case, knowledge of the naming and availability of the --wp-admin-theme color variant)
I actually think we need both names (like we do in SASS), basically this:
UI.borderFocus = UI.adminThemeColorDarker10;
The colors variables are important because they are what defines a "theme".
And the border variables are important because they are what defines the usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo. Valid point. I can do that in a follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving. I don't mind if the renaming happens separately. Thanks for the fix.
@youknowriad Thank you! 🙏 |
@ItsJonQ Something I'm wondering though. In SASS/CSS we do generate fallback colors for IE11. Do we need something like that or is it already taken care of? |
I actually wonder if this PR breaks the component in IE11 😬 |
@youknowriad Unfortunately, it does not automatically generate fallbacks. I can manually add one in a follow-up PR (working on it now) |
This update adjusts the color values retrieved from the JS
color()
function to use the--wp-admin-theme-color
CSS variable. This enables it to tap into values defined by a color scheme.Checklist:
Resolves: #23932