-
Notifications
You must be signed in to change notification settings - Fork 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
Calypso Color Schemes: Scope the accent variable for GB components. #99159
base: trunk
Are you sure you want to change the base?
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
I'd love your input on this draft PR. It's draft because it removes a couple of variables. But in my testing of Calypso and other instances, removing that one section that overrides Gutenberg components does not cause issues. But I'd appreciate your input in case there's a better fix. It is, for example, curious that this CSS bleed issue happens only with the default color scheme (the legacy blue), but not with other color schemes, like Sunrise. To be clear, Blueberry is the brand color for WordPress.com, it should be default there, and for new simple sites. Perhaps even new Atomic sites? But if you explicitly choose the classic color scheme, or another, that choice should technically work, which is the issue that's being fixed here. |
--wp-admin-theme-color: var(--color-accent); | ||
--wp-admin-theme-color-darker-20: var(--color-accent-60); |
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.
Are we intentionally making this change only to the default color scheme, but not to any of the others?
All of the other color schemes in this package are applying this change currently.
How is the "Default" one different?
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.
My understanding: this color scheme is not really used in Calypso, so that's how it's different. "Default" in Calypso is actually _fresh.css
.
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.
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.
This is intentional for now, insofar as I believe it solves the issue in the smallest way possible. This is also recognizing that Modern and the WordPress.com default brand color Blueberry both happen to match the default color that's already baked into the componentry itself. Incidentally that may also camouflage some issues. It's unclear if we should land #99033 first, then rebase this on top.
My understanding: this color scheme is not really used in Calypso, so that's how it's different. "Default" in Calypso is actually _fresh.css.
First off, that's interesting. _fresh.css is the legacy blue color, whereas _modern.css is the new Blueberry blue. _default.css I initially thought was the core default, but that's fresh. You can also see the admin-theme-fresh
class applied to the body tag of atomic admin sites, supplying these colors. Hence my comments in the files. Important to get right, of course.
That being said, the real issue here is that most of the calypso color schemes are scoped to CSS classes, i.e. .color-scheme.is-midnight { ... }
, whereas in _default.css, it's unscoped, i.e. :root { ... }
. That :root class, combined with the practice of reassigning one variable with another (--wp-admin-theme-color: var(--color-accent);
) is what's causing the bleed here.
It'd be nice if someone who had worked on the color schemes could help share any nuance, so I'm not missing anything.
Thanks for the review!
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.
It'd be nice if someone who had worked on the color schemes could help share any nuance, so I'm not missing anything.
cc @Automattic/lego who I believe worked on moving color scheme management to wp-admin recently.
I left a few comments that indicate my train of thought. TL;DR: I think this makes sense, but due to the fact that CSS is shared, we'll have to test this across:
|
Fixes #98823
Proposed Changes
This PR removes the following code from the _default color scheme, which is the scheme that runs on the WordPress.com hosting layer:
Note, this is not to be confused with the open WordPress core default color scheme, which is actually called "Fresh". That's this one:
That color scheme is still set by default for new Atomic sites, but for new Simple sites, the Modern scheme is set:
The issue at hand here is, that if you have the classic blue color scheme set, the one that uses the legacy WordPress blue, for the moment you still see the modern Blueberry color applied inside the site editor:
What you really should be seeing, and what this PR accomplishes, is the same accent color on the primary button, as is applied to other primary buttons in your admin. The classic blue:
The problem is, that the CSS above, assigned at the
:root
level, and gets included in this same unscoped form via the Help Center (click the (?) button in the top left of the site editor). Through its inclusion there, it bleeds into the WordPress admin itself, and overrides the local admin color scheme.So basically, WP.com hosting dashboard modern scheme overrides only parts of the wp-admin color scheme.
Testing Instructions
Pre-merge Checklist