Modern Color Scheme: Update to match wp-admin #92399
Conversation
matticbot
added
[Status] Needs Review
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. |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Hm, I dont see |
Hm, I didn't get why do we need to do |
Sorry 😅 I didn't get - should I activate it for both simple and atomic websites? UPDATE Oh, the theme activates not for the site, but for account, so it's common configuration |
|
Also, I was a bit confused - I have 3 tabs and initially it was hard to understand - which 2 of them is necessary for testing.
Actually, I am still not sure that I understand what necessary to be tested 😅 |
|
Also, it would be very helpful if you provide some screenshots, to understand what to expect and to understand what is the specific block. To make sure that I am testing right thing. |
There was a problem hiding this comment.
LGTM and confirm that styles now synced between Calypso and wp-admin 👍
5.1 Unfocused text and background colors for menu items and submenu items:
5.2 Unfocused text and background colors in the flyout when hovering over an expandable item like "Posts"
5.3. Unfocused text and background colors in the in-sidebar submenu when an expandable item is selected
5.4. Unfocused text and background colors for the currently selected items (e.g. Posts) and submenu items (e.g. All Posts)
5.5. Hover text and background colors for menu items and submenu items
5.6. Hover text and background colors in the flyout when hovering over an expandable item like "Posts"
5.7. Hover text and background colors in the in-sidebar submenu when an expandable item is selected
5.8. Hover text and background colors for the currently selected items (e.g. Posts) and submenu items (e.g. All Posts)
5.9. The "Collapse Menu" link, both unfocused and hovered
My mistake, thanks @nightnei. I was going off of my own site which I made atomic for these tests, and I still needed to activate the hosting settings to actually make it go atomic. Your site, I'm guessing, is already atomic, so you'll just want to hit the "Hosting" sidebar item that is probably already present. I've updated the testing instructions.
The wp-admin page is just for comparison, so we can confirm that the other two calypso-based views match visually. So you're testing the Calypso sidebar on something like Pages or Posts, in one tab, and the Nav Redesign sidebar (your atomic site with the Hosting menu item selected). As you test, you'll compare both of those tabs to the wp-admin tab. |
There was a problem hiding this comment.
Kudos for tackling this – it'll be a nice paper cut fix 👍
I left a comment to clarify whether we need both --color-sidebar-submenu-selected-hover-text and --color-navredesign-sidebar-submenu-selected-hover-text, or if we can do with just one.
I also noticed that the WordPress icon in the master bar doesn't have the same background color as the sidebar and the master bar. That's seems like something we should fix while we're at it.
| Calypso: Modern theme | Calypso: Classic dark theme | wp-admin: Modern theme |
|---|---|---|
| @@ -178,6 +179,7 @@ Uses custom theme highlight monochromatic palette for primary + accent | |||
| --color-navredesign-sidebar-menu-selected-text: var(--theme-text-color); | |||
| --color-navredesign-sidebar-submenu-selected-text: var(--theme-text-color); | |||
| --color-navredesign-sidebar-submenu-hover-text: #33f078; /* $menu-submenu-focus-text */ | |||
| --color-navredesign-sidebar-submenu-selected-hover-text: var(--color-sidebar-submenu-hover-text); | |||
There was a problem hiding this comment.
The distinction between --color-sidebar-submenu-selected-hover-text and --color-navredesign-sidebar-submenu-selected-hover-text is a little confusing.
I looked up the code history and found that the Nav Redesign variables were introduced in #90193. It's a little difficult to tell if adding those new variables was necessary or convenient… Knowing that might help inform us if we should add a single variable, or if adding both makes more sense.
Because all the CSS vars in the L632-L655 block in client/my-sites/sidebar/style.scss use the --color-navredesign prefix, I can see why you'd want to stick to that convention, @chad1008, but when there's no difference between --color-sidebar-submenu-selected-hover-text and --color-navredesign-sidebar-submenu-selected-hover-text, that does add some confusion.
@chad1008, what do you think?
There was a problem hiding this comment.
cc @nightnei here as well in case you have any insight from reviewing the other PRs
There was a problem hiding this comment.
It's a little difficult to tell if adding those new variables was necessary or convenient
I had the same thoughts
but when there's no difference between --color-sidebar-submenu-selected-hover-text and --color-navredesign-sidebar-submenu-selected-hover-text, that does add some confusion.
I had the same concern, but after thinking about it more - I made a decision that it's indeed very good idea to use an extra variable. I will try to express my thoughts: as you mentioned above - there are two different prefixes to distinguish styles, and if we reuse some variable - we would start mixing them, what is not good in case if we do some clean up or refactoring. Also it's potential issue for regression. I don't know, whether I managed to be clear... 😅
There was a problem hiding this comment.
There was a problem hiding this comment.
I used navredesign prefix since it was part of Nav Redesign project: pfsHM7-Ao-p2.
However, the naming was not ideal 
There was a problem hiding this comment.
So, basically to clarify which changes were introduced as part of Nav Redesign?
There was a problem hiding this comment.
I can see why you'd want to stick to that convention, @chad1008, but when there's no difference between --color-sidebar-submenu-selected-hover-text and --color-navredesign-sidebar-submenu-selected-hover-text, that does add some confusion.
Oh, it's definitely confusing! 😅
My rationale for using the navredesign variables is that those appear to be the ones that impact the Hosting/My Home view we're updating, while the "normal" variables without that prefix target the rest of Calypso, at least in practice for the items I've been working on.
This is part of what made this a more confusing process to update (and test, thanks again @nightnei!). The "Hosting/My Home" menu is basically a "Calypso in wp-admin" menu, because it's the one set of Calypso screens we still serve when a site owner has wp-admin set as their preferred interface. That separate menu, it turns out, has its own separate variables! yay!
I really like the issue @okmttdhr shared above. It would be great to standardize and simplify these variables across the different places they're being used.
There was a problem hiding this comment.
My rationale for using the
navredesignvariables is that those appear to be the ones that impact theHosting/My Homeview we're updating, while the "normal" variables without that prefix target the rest of Calypso, at least in practice for the items I've been working on.
This makes sense to me!
Because all the CSS vars in the L632-L655 block in client/my-sites/sidebar/style.scss use the --color-navredesign prefix, I can see why you'd want to stick to that convention, @chad1008, but when there's no difference between --color-sidebar-submenu-selected-hover-text and --color-navredesign-sidebar-submenu-selected-hover-text, that does add some confusion.
Something to consider is that if a value is used in a totally different UI, it will get more difficult to update it later.
The idea behind having a new prefix is for safety reasons; Nav Unification and Nav Redesign have different ideas about the color schema.
I agree that these things might increase complexity, so I think it would be ideal to redesign the naming conventions, taking into account where they are used and so on.
As stated in the Issue: https://github.com/Automattic/dotcom-forge/issues/7766, I believe that many "prefixed variables" could be retired by introducing Core color schemes as possible to their original specifications.
CCing: @fushar just in case... 🙏
There was a problem hiding this comment.
My rationale for using the navredesign variables is that those appear to be the ones that impact the Hosting/My Home view we're updating
Thanks for the clarification 👍
For reference, https://github.com/Automattic/dotcom-forge/issues/7884 has more details on the Hosting/My Home definition.
I still don't fully understand why we need separate variables to style those elements, but it makes sense to me to follow the existing convention and potentially address this in the future.
There was a problem hiding this comment.
Hi all! Thanks @okmttdhr for the ping. Yeah, we add the new navredesign variables specifically for the "Hosting" Calypso pages. We did that because the rest of Calypso (nav-unification) pages don't seem to match Core for some reason, and we don't want to break them in the context of Untangling project.
What is the goal of the PRs? Are we just fixing the "Hosting" pages or also the rest of Calypso pages, to match with the wp-admin scheme? Or also "fixing" all Calypso pages?
| @@ -166,6 +166,7 @@ Uses custom theme highlight monochromatic palette for primary + accent | |||
| --color-sidebar-menu-hover-text: var(--theme-text-color); | |||
|
|
|||
| /* Sidebar Hover - Nav unification */ | |||
| --color-sidebar-submenu-selected-hover-text: var(--color-sidebar-submenu-hover-text); | |||
There was a problem hiding this comment.
Let's move this below --color-sidebar-submenu-hover-text
There was a problem hiding this comment.
good idea. I'm applying this change to other schemes that will benefit of it as well.
|
@chad1008, WDYT about the background color for the WordPress icon in the master bar? See my previous comment for details |
|
Ironically @fredrikekelund, i typed this message earlier and apparently neglected to hit send. I'd just noticed that when I saw your more recent question.
This question got lost in the shuffle at first, sorry @fredrikekelund. I did a bit of digging and it looks like this is outside of the color schemes themselves. That button is getting an I need to explore a little further to see if that's expected, but I'm thinking this might be better fixed in a separate PR, since it's outside of the actual color schemes. Thoughts? Either way, @alshakero I know it was a couple of years back that you worked on this section of the codebase, but do you have any recollection/insight as to what the desired styling/active state is for the WordPress logo/"My Sites" button on desktop browsers? It looks/feels to me like it's "active" more than it should be, but I'm curious about what you might recall 😄 |
If that's the case, then I agree. Let's land these 👍 |
|
Merged and deployed with #92397 |
github-actions
bot
removed
the
[Status] Needs Review
Part of 7884-gh-Automattic/dotcom-forge
Proposed Changes
Updates the Modern color scheme to address discrepancies from wp-admin, for a more consistent user experience.
Why are these changes being made?
An issue recently highlighted some disconnects that have developed between our unified/redesigned sidebars and the wp-admin styles for various color schemes. This PR is addresses those for the Modern color scheme.
Note
Because the schemes depend on the variables provided by
client/my-sites/sidebar/style.scss, this PR is branched off of #92397.If any changes are recommended for
client/my-sites/sidebar/style.scss, please leave a note on that PR instead of this one, so I can make changes there, and then rebase the individual scheme PRs.Testing Instructions
Important Some items have received changes to their code that won't be reflected visually, because the only change was renaming a variable. That means comparing to production/trunk can be misleading, because a changed sidebar might look just like production, but the change is still needed for the new variables in the sidebar styles to work.
Instead of direct production/trunk comparisons of a link like (for example) My Sites under Hosting, focus on comparing the behavior of the types of links you're looking at. When My Sites on the Hosting sidebar is the currently selected menu item, it should behave the same way and have the same color/hover effect as, for example All Posts under Posts, when it's selected... basically, when testing this one, avoid asking:
Instead, ask:
There are three views we care about:
/home/[site-url], where you'll find a Calypso interface with the Hosting section open to the My Home submenu item6.1. Unfocused text and background colors for menu items and submenu items
6.2. Unfocused text and background colors in the flyout when hovering over an expandable item like "Posts"
6.3. Unfocused text and background colors in the in-sidebar submenu when an expandable item is selected
6.4. Unfocused text and background colors for the currently selected items (e.g. Posts) and submenu items (e.g. All Posts)
6.5. Hover text and background colors for menu items and submenu items
6.6. Hover text and background colors in the flyout when hovering over an expandable item like "Posts"
6.7. Hover text and background colors in the in-sidebar submenu when an expandable item is selected
6.8. Hover text and background colors for the currently selected items (e.g. Posts) and submenu items (e.g. All Posts)
6.9. The "Collapse Menu" link, both unfocused and hovered
The hover states of a currently selected submenu item like "All Posts" and the "Collapse Menu" link were where I found the most nuance/variation from one scheme to the next.