Site Editor: Apply the user's admin color scheme#78397
Conversation
0fc11f7 to
231eebb
Compare
|
Size Change: +1.18 kB (+0.01%) Total Size: 8.44 MB 📦 View Changed
|
|
Flaky tests detected in e015be2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27114741231
|
|
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. |
ccce419 to
f733f13
Compare
|
Was also about to share similar feedback to @jameskoster re. using the correct tokens. The |
9c6817c to
32d2f18
Compare
|
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.
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.
Yes, that's my impression as well 😅 there's nothing stopping us to provide the wrong areas, and some areas are overlapping across viewports, even before this change.
This PR does not try to solve the strictness as it seems to be much bigger refactor.
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
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!
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?
| 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.
…color-4 # Conflicts: # packages/edit-site/src/components/layout/index.js
fushar
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.
Thanks for all your comments patience (and other reviewers)! I am going to merge this soon. I believe it's in a good shape now.
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?
Yeah, I created a new issue as noted here: https://github.com/WordPress/gutenberg/pull/78397/changes/BASE..87c14037c5d98141b3274f34ea4952dc5c17cb06#r3370732998
| color: $white; | ||
| color: var(--wpds-color-fg-interactive-neutral-active); |
There was a problem hiding this comment.
I created a separate issue instead to track this: #79001
| 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.
Yes, that's my impression as well 😅 there's nothing stopping us to provide the wrong areas, and some areas are overlapping across viewports, even before this change.
This PR does not try to solve the strictness as it seems to be much bigger refactor.
|
@fushar Thank you for taking this to the finishing line! There are a couple more follow-up tasks / PRs that we discussed, just to make sure they're not left behind:
Do you mind creating issues for it ? Would you also have time to work on follow-ups personally? |
@ciampo: I'll answer below:
Yeah, I totally missed that! Fixed in #79056.
Hmm I'm not sure if I have any better idea for this. As explained in #78397 (comment), this PR does not really make things worse; it's the inherent limitation from the beginning. I think it's out of scope and I don't have bandwidth to try to "fix" this. |
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