Skip to content

Conversation

@SirLouen
Copy link
Member

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.

@github-actions
Copy link

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: SirLouen <sirlouen@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the [Package] Base styles /packages/base-styles label Jan 30, 2026
@SirLouen SirLouen added [Type] Enhancement A suggestion for improvement. [Package] Theme /packages/theme and removed [Package] Base styles /packages/base-styles labels Jan 30, 2026
@SirLouen SirLouen self-assigned this Jan 30, 2026
@SirLouen SirLouen requested review from aduth and youknowriad January 30, 2026 15:22
...restProps
}: React.ComponentProps< typeof ThemeProvider > ) {
const primary = getAdminThemePrimaryColor();
const bg = getAdminThemeBgColor();
Copy link
Member

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.

Copy link
Member Author

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 {
Copy link
Member

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.

Copy link
Member Author

@SirLouen SirLouen Jan 30, 2026

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 ];
Copy link
Member

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.

Copy link
Member Author

@SirLouen SirLouen Jan 30, 2026

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

Copy link
Member Author

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:

packages/base-styles/index.js

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);
Copy link
Member

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.

@aduth aduth requested a review from a team January 30, 2026 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Theme /packages/theme [Type] Enhancement A suggestion for improvement.

2 participants