Site Editor: Apply the user's admin color scheme#78397
Conversation
github-actions
Bot
added
[Package] Edit Site
fushar
force-pushed
the
admin-bar-in-editor-color-4
branch
from
0fc11f7 to
231eebb
Compare
|
Size Change: +1.2 kB (+0.01%) Total Size: 8.2 MB 📦 View Changed
|
|
Flaky tests detected in 6b8ef7e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27010619817
|
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @lucasmendes-design. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
fushar
added
[Type] Feature
fushar
force-pushed
the
admin-bar-in-editor-color-4
branch
4 times, most recently
from
ccce419 to
f733f13
Compare
|
Was also about to share similar feedback to @jameskoster re. using the correct tokens. The |
fushar
force-pushed
the
admin-bar-in-editor-color-4
branch
from
9c6817c to
32d2f18
Compare
|
Got it. Thanks, Jay. |
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
This is really exciting to see the theming being applied in the site editor 🎉 Code is looking to be in good shape. The main blockers for me from my comments are verifying remaining base styles color variable usage, and (if feasible) a more resilient way to condition around rendering the mobile content area.
| } | ||
| } | ||
|
|
||
| .edit-site-layout__mobile-content { |
There was a problem hiding this comment.
It seems to be an existing issue, but I notice the background color bleeds through a bit on the right-hand side of the content wrapper due to a padding on a container element. Not a blocker since it's pre-existing (except strangely the background bleed extends to the header region now as well).
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Hmmm, somehow I can't reproduce it locally 🤔
There was a problem hiding this comment.
Tested on gutenberg.run, was able to reproduce:
This happens because the edit-site-sidebar__screen-wrapper element (responsible for showing a scrollbar) is not wrapped in the ThemeProvider that overrides the bg color to white:
gutenberg/packages/edit-site/src/components/layout/index.js
Lines 175 to 185 in 6dcd757
but, as @aduth mentions, this can be fixed in a follow-up PR.
There was a problem hiding this comment.
Related: we should also change the scrollbar color here:
gutenberg/packages/edit-site/src/components/sidebar/style.scss
Lines 38 to 41 in 35ba775
to use --wpds-color-bg-thumb-neutral-weak instead of hardcoded gray (and --wpds-color-bg-thumb-neutral-weak-active for hover styles, if possible)
There was a problem hiding this comment.
Ah, yeah, that would explain it: I have scrollbars set to always show in my system settings. While we shouldn't associate narrow screen size to mobile devices or even that mobile devices necessarily don't show a scrollbar, it seems like it'd be pretty rare for this to be seen in practice. Good to fix (separately) all the same.
| color: $white; | ||
| color: var(--wpds-color-fg-interactive-neutral-active); |
There was a problem hiding this comment.
I'm curious about the remaining color references in this file. Are they still valid or should they be using tokens as well? I might have expected us to be able to eliminate the @use "@wordpress/base-styles/colors" as *; at the top of the file altogether.
gutenberg/packages/edit-site/src/components/layout/style.scss
Lines 113 to 115 in 87c1403
There was a problem hiding this comment.
I've audited the remaining usages here: 5879262. Here's what I updated:
- Text:
$gray-900->--wpds-color-fg-content-neutral - Background:
$white->--wpds-color-bg-surface-neutral-strong - Button text:
$gray-900->--wpds-color-fg-interactive-neutral
There's a few color references which I didn't touch. For examples:
It does not really come from the admin color schemes, and there's no WPDS token for $gray-300, so I didn't update them.
There was a problem hiding this comment.
there's no WPDS token for $gray-300, so I didn't update them.
There won't always be a perfect match, but I think that --wpds-color-stroke-surface-neutral should be the right token here, perhaps?
Button text: $gray-900 -> --wpds-color-fg-interactive-neutral
We shouldn't really need to override color and most aspects of the button (looking at the .edit-site-layout__view-mode-toggle.components-button styles). Can we remove most of those and rely on default button styling?
Or, at most, add a TODO and follow-up separately once the @wordpress/ui Button is ready for consumption in Gutenberg?
cc @jameskoster
There was a problem hiding this comment.
Thanks for checking! Hmm. I would say both proposals are kind of a bit out of scope of this PR. The first one ($gray-300 for the border) will make the PR bigger as we need to update it in more places so that all borders will have consistent color 😄 and I feel the current color is good for all schemes.
And the second one looks like CSS cleanup to me.
I think we should do that as separate PR instead, what do you think?
There was a problem hiding this comment.
there's no WPDS token for $gray-300, so I didn't update them.
There won't always be a perfect match, but I think that --wpds-color-stroke-surface-neutral should be the right token here, perhaps?
I retracted my previous reply; yeah I think this is the right thing to do. Applied via e5dbc71, thanks!
There was a problem hiding this comment.
We shouldn't really need to override color and most aspects of the button (looking at the
.edit-site-layout__view-mode-toggle.components-buttonstyles). Can we remove most of those and rely on default button styling?
I tried to do this. But I think (almost) all of such overrides are required. For example, the .edit-site-layout__view-mode-toggle.components-button button is the one that renders the W logo at top-left. The color must be white in all schemes and not themed.
Some of them are also required because we render buttons in the sidebar, like this issue here #66166.
I'd keep them for now to not make more changes in this PR 😄
There was a problem hiding this comment.
We should try to clean up those styles, but in a separate follow-up PR.
For example, for the W logo, we should probably just use a vanilla a and add styles as necessary (since it's such a custom piece of UI)
There's a few color references which I didn't touch
Out of curiosity, are there any more references that haven't been refactored yet?
There was a problem hiding this comment.
Out of curiosity, are there any more references that haven't been refactored yet?
To the best of my knowledge, now I think I've fixed them all!
There was a problem hiding this comment.
Just to note, I believe there are quite a few instances of $gray-300 applied to borders in Gutenberg/@wordpress/components. If we're going to update some of them here then we should ideally follow up quickly to update all the others to ensure consistency.
| { areas.content && | ||
| areas.mobile?.type !== | ||
| areas.sidebar?.type ? ( |
There was a problem hiding this comment.
I'm a little less familiar with the inner workings of the site editor routing and its "areas", but it feels a bit fragile to compare component types like this. It feels like we can only trust the comparison out of convention for how components are reused within the package, but it's not a strong guarantee. Ideally we could have a stronger indicator, like a dedicated property on the route, a separate area, or maybe something with React context.
There was a problem hiding this comment.
Yeah, this one is the trickiest in the whole changes. 😅
We can come up with better conditionals but it requires a refactor. I gave it a try here: f9ca040. Here, we split mobile area into either mobileSidebar and mobileContent:
- Use
mobileSidebarif the content is to be rendered as sidebar (with the themed background) - User
mobileContentif the content is to be rendered as content (with white background)
There's one more case: mobile area is also rendered when canvas is in edit mode, so we had so many component wrappers like MobileIdentityView just for branching. I noticed it's basically always the same, semantically, as areas.preview, so we can eliminate all the wrappers and just render it in the canvas !== 'edit' else-case.
Can you check. It's larger refactor that I had hoped, but the changes look clean, make sense, and I've tested clicking around and didn't see regressions. 😄
There was a problem hiding this comment.
That seems sensible enough to me, especially if we already differentiated between sidebar and content to begin with why would mobile overrides collapse back to a single amorphous component. But again I don't have a ton of experience to weigh site editor API changes like this, or if there's perhaps backwards-compatibility concerns to be aware of (since your proposal eliminates areas.mobile from the API). You may want to double-check if this is part of an extensible public API or invite feedback from someone more versed in the site editor.
There was a problem hiding this comment.
You may want to double-check if this is part of an extensible public API
I believe it's private API. It's returned by useLocation(), which is never exposed as public API by @wordpress/router. All references to areas.mobile were internal site editor routes.
or invite feedback from someone more versed in the site editor.
Asking @youknowriad who touched many parts of this logic, what do you think Riad? 😄
There was a problem hiding this comment.
why would mobile overrides collapse back to a single amorphous component.
Because on mobile, there's no "sidebar" in the site editor, there's always a single area and it renders either the sidebar or the content of the page depending on the url/state.
That said, this is very old for me and I don't believe that I have a lot more context that you do today.
There was a problem hiding this comment.
And yes, all of this is private APIs, third-parties can't add routes or things like that yet.
There was a problem hiding this comment.
OK, thanks for the context, makes sense. Now with this PR, a mobile screen should still know whether it is rendering a "sidebar" (which is themed by the color scheme) or a "content" (which should force the background to white as we currently do before this change). So I think the proposed change still stands :)
There was a problem hiding this comment.
@aduth @youknowriad can you confirm if this thread needs any more action, or if we can assume it's resolved?
There was a problem hiding this comment.
Yes, I'm a lot less concerned if it's a private API, and the updated logic for conditioning on mobileContent and mobileSidebar looks a lot more robust 👍
| } | ||
| } | ||
|
|
||
| .edit-site-layout__mobile-content { |
There was a problem hiding this comment.
Hmmm, somehow I can't reproduce it locally 🤔
| color: $white; | ||
| color: var(--wpds-color-fg-interactive-neutral-active); |
There was a problem hiding this comment.
I've audited the remaining usages here: 5879262. Here's what I updated:
- Text:
$gray-900->--wpds-color-fg-content-neutral - Background:
$white->--wpds-color-bg-surface-neutral-strong - Button text:
$gray-900->--wpds-color-fg-interactive-neutral
There's a few color references which I didn't touch. For examples:
It does not really come from the admin color schemes, and there's no WPDS token for $gray-300, so I didn't update them.
| { areas.content && | ||
| areas.mobile?.type !== | ||
| areas.sidebar?.type ? ( |
There was a problem hiding this comment.
Yeah, this one is the trickiest in the whole changes. 😅
We can come up with better conditionals but it requires a refactor. I gave it a try here: f9ca040. Here, we split mobile area into either mobileSidebar and mobileContent:
- Use
mobileSidebarif the content is to be rendered as sidebar (with the themed background) - User
mobileContentif the content is to be rendered as content (with white background)
There's one more case: mobile area is also rendered when canvas is in edit mode, so we had so many component wrappers like MobileIdentityView just for branching. I noticed it's basically always the same, semantically, as areas.preview, so we can eliminate all the wrappers and just render it in the canvas !== 'edit' else-case.
Can you check. It's larger refactor that I had hoped, but the changes look clean, make sense, and I've tested clicking around and didn't see regressions. 😄
…color-4 # Conflicts: # packages/admin-ui/CHANGELOG.md
|
All right, I've tested this PR hundreds of times :) I think this is good. Here's a series of screenshots from desktop and mobile resolutions to showcase the changes (and to make sure the mobile refactor still works): Desktop
Mobile
|
|
Thanks for working on this. I believe this PR will ultimately close #53430. I may not fully understand this PR, but one concern I have is backward compatibility. This PR works well with the latest WordPress trunk, meaning the core version including changeset 62454. However, the Gutenberg plugin also supports older WP versions. Therefore, if I test this PR with WordPress 6.9 or 7.0, I see a difference in background color on boot-based pages. 
To resolve this, we could implement changes similar to changeset 62454 in Gutenberg, but this would likely require extensive modifications. As an alternative approach, if the WordPress version is 7.0 or lower, we might consider reverting only the background color of the boot page to its previous setting. For example, somthing like this: Detailsdiff --git a/lib/compat/wordpress-7.1/admin-color-schemes.php b/lib/compat/wordpress-7.1/admin-color-schemes.php
new file mode 100644
index 00000000000..0296d20b6e7
--- /dev/null
+++ b/lib/compat/wordpress-7.1/admin-color-schemes.php
@@ -0,0 +1,31 @@
+<?php
+/**
+ * WordPress 7.1 compatibility functions for the Gutenberg
+ * editor plugin changes related to admin color schemes.
+ *
+ * @package gutenberg
+ */
+
+/**
+ * Prints a background fallback for the boot-based admin screens.
+ *
+ * The admin on WordPress < 7.1 still uses the legacy color schemes, which no
+ * longer match the boot pages' updated background colors. Apply the legacy
+ * colors to the boot pages' background only so the two stay consistent.
+ */
+function gutenberg_print_legacy_admin_colors_styles() {
+ if ( version_compare( get_bloginfo( 'version' ), '7.1.0', '>=' ) ) {
+ return;
+ }
+ ?>
+ <style id="gutenberg-legacy-admin-colors">
+ .admin-color-blue .boot-layout { background: #52accc; }
+ .admin-color-coffee .boot-layout { background: #59524c; }
+ .admin-color-midnight .boot-layout { background: #363b3f; }
+ .admin-color-ectoplasm .boot-layout { background: #523f6d; }
+ .admin-color-ocean .boot-layout { background: #738e96; }
+ .admin-color-sunrise .boot-layout { background: #cf4944; }
+ </style>
+ <?php
+}
+add_action( 'admin_print_styles', 'gutenberg_print_legacy_admin_colors_styles' );
diff --git a/lib/load.php b/lib/load.php
index 2173e9e35c8..26e010d4deb 100644
--- a/lib/load.php
+++ b/lib/load.php
@@ -130,6 +130,7 @@ if ( class_exists( '\WordPress\AiClient\AiClient' ) ) {
// WordPress 7.1 compat.
require __DIR__ . '/compat/wordpress-7.1/classic-block.php';
+require __DIR__ . '/compat/wordpress-7.1/admin-color-schemes.php';
// Experimental features.
require __DIR__ . '/experimental/admin-bar-in-editor/load.php'; |
|
@t-hamano Thanks for checking!
Hmm don't think this counts a regression for WP < 7.1 because on those versions, we will already see different background, which is (always) black because it's not themed: 
Does this really count as regression which needs backcompat? I'm pretty new to this so I might be wrong 😄 |
You're right, inconsistencies in background color can already occur in trunk 😅 This pull request does not introduce any regressions and, in fact, includes improvements. |
| const hasMobileAreas = | ||
| areas.mobileSidebar || areas.mobileContent || areas.preview; |
There was a problem hiding this comment.
I see that mobile got split into mobileSidebar + mobileContent.
Just to confirm: the item routes (template-item, page-item, pattern-item, template-part-item) no longer define any mobile* area, so on mobile they rely entirely on areas.preview being rendered in the canvas === 'edit' branch.
That leaves two edges that I'm not able to confirm if they are covered:
canvas !== 'edit'on an item route:mobileContentandmobileSidebarare bothundefined, soSidebarContentrenders empty. Iscanvasguaranteed to be'edit'for these routes on mobile? Previously, themobilearea rendered theEditor/Unsupportedscreen unconditionally.- Loading state:
hasMobileAreas = areas.mobileSidebar || areas.mobileContent || areas.preview. For item routes,areas.previewresolves tonulluntilsiteDataloads, sohasMobileAreasis falsy and the entire mobile block (site hub included) renders nothing during load. Routes that return<></>frommobileSidebarkeep the hub visible in the same window.
If both paths are genuinely unreachable, could we add a short comment on the item routes (e.g. "mobile is handled via preview in edit canvas only") so this contract is explicit and we don't regress it later?
There was a problem hiding this comment.
Is canvas guaranteed to be 'edit' for these routes on mobile?
Yep, I checked and those "item routes" are only reachable on mobile at canvas=edit. For example:
page-item:
template-item:
pattern-item / template-part-item:
I added comments to clarify things: cd48d10.
Loading state
Returning null and <></> is equivalent here in this case fortunately, as in these canvas=edit routes, we don't render the mobile site hub component. So I believe we're good here!
There was a problem hiding this comment.
Mmm, this "soft contract" about how routes define their areas and canvas modes feels particularly fragile, but I suppose that fragility already existed. It seems like there's a high risk to mistakenly cause blank rendering if someone doesn't specify canvas=edit in a route that doesn't define mobileContent or mobileSidebar. And it's the sort of issue that would be really easy to miss since most people probably aren't testing regularly on mobile. No amount of inline comments on the routes themselves would help this, it should be the responsibility of the Layout.
But at least as far as I understand, it's always worked this way, and hasn't gotten any worse with the changes here.
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
We should probably update the PR description to mention https://core.trac.wordpress.org/changeset/62454 and a little bit more explanation about the change overall.
fushar
left a comment
fushar
left a comment
There was a problem hiding this comment.
We should probably update the PR description to mention https://core.trac.wordpress.org/changeset/62454 and a little bit more explanation about the change overall.
Yep, I updated the PR description, thanks for the reminder!
| const hasMobileAreas = | ||
| areas.mobileSidebar || areas.mobileContent || areas.preview; |
There was a problem hiding this comment.
Is canvas guaranteed to be 'edit' for these routes on mobile?
Yep, I checked and those "item routes" are only reachable on mobile at canvas=edit. For example:
page-item:
template-item:
pattern-item / template-part-item:
I added comments to clarify things: cd48d10.
Loading state
Returning null and <></> is equivalent here in this case fortunately, as in these canvas=edit routes, we don't render the mobile site hub component. So I believe we're good here!
| color: $white; | ||
| color: var(--wpds-color-fg-interactive-neutral-active); |
There was a problem hiding this comment.
Out of curiosity, are there any more references that haven't been refactored yet?
To the best of my knowledge, now I think I've fixed them all!
| &:hover, | ||
| &:focus { | ||
| background: var(--wpds-color-bg-interactive-brand-strong-active); | ||
| color: var(--wpds-color-fg-interactive-brand-strong); |
There was a problem hiding this comment.
| color: var(--wpds-color-fg-interactive-brand-strong); | |
| color: var(--wpds-color-fg-interactive-brand-strong-active); |
There was a problem hiding this comment.
Ah, I always missed the semantic 🤦 Thanks, updated: 6b8ef7e
| } | ||
| } | ||
|
|
||
| .list-view-appender .block-editor-inserter__toggle { |
There was a problem hiding this comment.
The styles are scoped under .list-view-appender — if the toggle can also render on other themed sidebar screens, we should consider covering them as well (maybe adding those styles in a more global palce?)
There was a problem hiding this comment.
This toggle is only rendered here, so I don't think we need to move this to a more global place for now. But I dropped the .list-view-appender scope as it's a bit redundant.
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
I'm going to preemptively approve this since this is looking to be in good shape overall and has gone through quite a few iterations. I left a few more comments that would be good to consider, and it sounds like we might have some more work to do on the color tokens side of things. If we're considering those as out of scope, can we at least track that effort somewhere?
| color={ { | ||
| bg: CONTENT_BACKGROUND, | ||
| } } |
There was a problem hiding this comment.
Minor, maybe performance-impacting: If we made the entire object the constant value, it would be much easier for React to reconcile that the props haven't changed.
| content: <SidebarIdentity />, | ||
| preview: <Editor />, | ||
| mobile: <MobileIdentityView />, | ||
| mobileContent: <SidebarIdentity />, |
There was a problem hiding this comment.
There are stale references to mobile in this file that we'll want to update to reflect these changes:
| const hasMobileAreas = | ||
| areas.mobileSidebar || areas.mobileContent || areas.preview; |
There was a problem hiding this comment.
Mmm, this "soft contract" about how routes define their areas and canvas modes feels particularly fragile, but I suppose that fragility already existed. It seems like there's a high risk to mistakenly cause blank rendering if someone doesn't specify canvas=edit in a route that doesn't define mobileContent or mobileSidebar. And it's the sort of issue that would be really easy to miss since most people probably aren't testing regularly on mobile. No amount of inline comments on the routes themselves would help this, it should be the responsibility of the Layout.
But at least as far as I understand, it's always worked this way, and hasn't gotten any worse with the changes here.
10 participants
What?
Makes the Site Editor's sidebar and page shell (chrome) to follow the user's WordPress admin color scheme instead of always rendering a fixed dark barkground.
Why?
This is a continuation of #78331, as per the suggestion I made the changes directly via WPDS tokens +
ThemeProviderrather than overriding in the experiment SCSS file.How?
Firstly the editor chrome color is produced by the WPDS ramp algorithm, which only emits colors in specific luminance bands and cannot produce arbitrary colors. So, I had to come up with new a color set for the existing schemes, and updated Core's to match them: https://core.trac.wordpress.org/changeset/62454.
Then, we extract
getAdminThemeColorsfrom@wordpress/bootto@wordpress/admin-ui, so both the Site Editor (@wordpress/edit-site) andbootcan share one implementation. It now also includes thebgcolor in addition toprimary. Previously,boothardcoded the sidebar background to#1d2327; it's now based on the admin color scheme.@wordpress/edit-sitenow passes theme colors toThemeProvider. Finally, we convert the hardcoded colors inedit-site's component SCSS files to the correct WPDS tokens semantically.There's also a bit complication. Sidebar area should be rendered with themed background color, while content area should be rendered with white (
#fff) background. On desktop, this is not a problem because we definesidebar,content, andpreviewareas, so we can wrap each of them with the correct background color via<ThemeProvider>. However, for mobile viewport, we only definemobilearea. So, we split it into two:mobileSidebarandmobileContent, mirroring existing areas. Oncanvas=editon mobile, we renderpreview, matching desktop.Limitations
Site Editor v2 (
/wp-admin/admin.php?page=site-editor-v2) is EXCLUDED for now. The page does not add the color scheme to the<body>'sclass. I'm not sure if that's intended? It does work correctly on otherbootpages like Appearance -> Fonts.Testing Instructions
Test the Site Editor (v1) with various admin color schemes (set via Users -> Profile). Also test on various screen widths to test mobile behavior.
Test also
bootpages such as Appearance -> Fonts.Screenshots
Appearance -> Fonts
(Check the right and bottom sides; they now follow the background color instead of black.)
Site Editor
Use of AI Tools
I used Claude Code with Opus 4.7 to help replace the hardcoded colors with appropriate WPDS token variables.
cc: @lucasmendes-design