Masterbar: Update colors so they match Modern (default) color scheme. #98542
Conversation
jasmussen
added
the
[Type] Enhancement
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
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. |
matticbot
added
the
[Status] Needs Review
| --color-masterbar-text: #fff; // From "Modern" color scheme. | ||
| --color-masterbar-icon: #fff; // From "Modern" color scheme. |
There was a problem hiding this comment.
Any reason not to use var(--studio-white) for these? It maps to this value.
There was a problem hiding this comment.
Excellent question. I thought about this, but had the feeling that this is the Modern color scheme, it's not any colo scheme from the Studio colors. I'd appreciate input from @Automattic/dotcom-design, as it's trivial for me to update.
Thank you for the review!
There was a problem hiding this comment.
Good catch. @jasmussen do you know where the color schemes are stored? That could help us understand if the values are always hard-coded or not.
There was a problem hiding this comment.
Yes, they are stored here: https://github.com/Automattic/wp-calypso/tree/trunk/packages/calypso-color-schemes/src/shared/color-schemes
But it's a bit of a mess, with what at a quick glance looks like a ton of circular variables, an almost literal rabbit hole. Perhaps it's because I'm fresh to the latest iteration of the codebase, but --color-sidebar-submenu-selected-text: var(--color-text-inverted); is a mystery to me. There are also situational overrides and separate concessions per section. For example for this PR I had to also edit this file: https://github.com/Automattic/wp-calypso/blob/trunk/client/my-sites/sidebar/style.scss
There was a problem hiding this comment.
I wonder if it's possible to import color values from Gutenberg, and use those instead of these custom vars?
There was a problem hiding this comment.
Coming back to this, Calypso imports the core base-styles, so I can import base-styles/color giving me access to the range of $white, $black, and $gray-100 to $gray-900. But it's not clear that's worth it.
You're probably referring to these colors:
Those are present in the admin contexts, but it's not clear to me those are available in the expanded contexts (/me, /sites, /read, etc.) I think it's for that reason that some place in the codebase, we have this remapping:
--wp-admin-theme-color: var(--color-accent);
I'm genuinely unsure what the best path forward is, but I'm increasingly tempted to merge this as is and follow up. What do you think?
There was a problem hiding this comment.
I wouldn't go that way without a general plan forward for color schemes.
Right now things can get really thorny with the existing Calypso color schemes in place. Especially when we aim for consistency in wp-admin too.
There was a problem hiding this comment.
Thanks for looking. I hear this as an argument to not use Studio colors, yes? If yes, I'll hardcode the blueberry again!
There was a problem hiding this comment.
I was talking about core colors and not studio colors. Studio colors make sense to be used instead of hardcoded colors, they're used broadly across Calypso already.
I was mostly saying that if we want to use core colors, we might want to do it more broadly, and it will be a gigantic effort across the app.
There was a problem hiding this comment.
Just in case we wanted to do so, we could look at importing @wordpress/base-styles/variables and @wordpress/base-styles/mixins and use the admin-scheme mixin
|
The new version looks great 👌 |
|
CC: @Automattic/team-calypso in case you have thoughts, specifically on this question. |
|
It works for me, @jasmussen ! Thanks for working on this 🚀 |
|
Default is the WordPress core default. But Modern is the default that gets set for any new site created on WordPress.com. Default also uses the same Blueberry color that is the default WordPress.com brand color. Because of those two de-facto defaults, (despite the core term "default"), the default colors in the rest of the admin should match. What do you think? |
Makes sense to me, and I've confirmed "Modern" gets assigned by default 👍 I guess we should rename "Default" to something else then. As a user, I'm confused I end up with another color scheme by default 😅 |
"Classic". How does this fit with untangling work, though? And does it have to be part of this PR or can it be separate? The blueberry is already de-facto default, right now the adminbar is just a mix of colors. Most importantly: I'm unsure about the answer to this question. Should I disconnect the blueberry for the modern color scheme, hard-code it again to #3858e9 instead of the studio reference? Or keep this as is and merge? |
This isn't something for this PR obviously 😉 Let's discuss it separately.
I'm fine to keep as-is, as long as we have the comment. Thanks! |
github-actions
bot
removed
the
[Status] Needs Review
|
Appreciate all the gut checks. Now merged, and will go through the deploy process. I hope and expect future PRs to hopefully go through more smoothly! |
Related to https://github.com/Automattic/dotcom-forge/issues/8519
Proposed Changes
Updates masterbar colors to match the Modern color scheme, which is the default admin color scheme.
Before
After
Why are these changes being made?
There are color scheme colors from individual site installs, and then global Calypso styles. Over time, colors have drifted out of sync, likely as default admin color schemes have been updated, and changed.
This PR fixes issues with the Modern color scheme (there was a contrast issue with the unread bell), and syncs up the color schemes applied to Calypso (Reader, Me, Sites) so they match the default Modern color scheme.
That specifically includes updating the grays. The default shades of gray in the WordPress design system (as defined here) are not yet in Color Studio, and must therefore be defined inline. CC: @Automattic/dotcom-design
Testing Instructions
Apply the PR, then test the wp-admin sections, such as Users > Profile, as well as Reader, Me, and Sites, to see if the colors of the adminbar/masterbar match.
Pre-merge Checklist