-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Theme Provider: Unify color schemes using CSS custom properties #75096
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
base: trunk
Are you sure you want to change the base?
Theme Provider: Unify color schemes using CSS custom properties #75096
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| ...restProps | ||
| }: React.ComponentProps< typeof ThemeProvider > ) { | ||
| const primary = getAdminThemePrimaryColor(); | ||
| const bg = getAdminThemeBgColor(); |
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 feel like this background handling should probably happen in Root and not here. It makes sense to me that the primary color should always correspond to the user's color preference, but there's a lot more variability in background colors (white, accent, etc.) that we wouldn't want to link the default background to the menu color.
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.
Yes, could be an option.
| * @param propertyName The CSS custom property name (e.g., '--wp-admin-color-highlight'). | ||
| * @return The property value or undefined if not set. | ||
| */ | ||
| function getCSSCustomProperty( propertyName: string ): string | undefined { |
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.
Separately, I think ThemeProvider should have built-in support for passing var(--foo) as color values, with this resolution happening inside the component.
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.
Interesting, I was also struggling in my mind, with this idea because of the sass limitations, you mean something like
resolveCSSVariable( 'var(--primary)' )To return #007cba
Something like:
const match = value.match( /^var\(\s*(--[\w-]+)(?:\s*,\s*(.+))?\s*\)$/ );
const propertyName = match[ 1 ];
const fallback = match[ 2 ];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 mean that we could do something like:
<ThemeProvider color={ { bg: 'var(--wp-admin-color-menu-background)' } } />...and it would "just work".
But I tinkered with this idea a bit locally and it's kinda tricky, because ThemeProvider has to be adaptive to its rendered context (i.e. where it is in the DOM hierarchy since CSS properties cascade and be overridden by DOM ancestors), whereas here you can be sure that the property is defined on document.body.
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.
Yep, it seems that it will require at least an extra render before getting the property we need to define the right variable to display. I'm still trying to figure out why the colors were included manually. I think that @youknowriad holds the key to this knowledge in #23048 (prob forgot, a little old) and you added this
const THEME_PRIMARY_COLORS = new Map< string, string >( [
[ 'light', '#0085ba' ],
[ 'modern', '#3858e9' ],
[ 'blue', '#096484' ],
[ 'coffee', '#46403c' ],
[ 'ectoplasm', '#523f6d' ],
[ 'midnight', '#e14d43' ],
[ 'ocean', '#627c83' ],
[ 'sunrise', '#dd823b' ],
] );
Quite recently in #74011
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.
Judging by the conversation context in #23048 it feels that back in the day GB was significantly disconnected from Core, prob back then Gutenberg Core Committers did not even had Core Committing access, so lending the whole color palette was the fastest way to proceed.
Is interesting to see this file history now:
Many colors in the scheme were being exported.
| body.admin-color-sunrise { | ||
| @include admin-scheme(#dd823b); | ||
| body { | ||
| --wp-admin-theme-color: var(--wp-admin-color-highlight); |
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 curious why it wasn't implemented this way in the first place 🤔 Maybe we're missing some historical context. I'll add some reviewers who may be able to shed some more light, or maybe we can review the original discussions behind this implementation. My first thought was maybe to avoid directly tying to WordPress internals, but we're already doing that with relying on the body class names.
In the series #75053
Why?
Instead of building from the same color scheme repeated over and over again through different parts of the code, why not unifying them?
How?
This is not final, I'm playing around with the idea of exposing certain parameters from Core:
See this Core PR: WordPress/wordpress-develop#10825
And fetching them unified from there.