Skip to content
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

Color Schemes: update sidebar styles to allow more specificity #92397

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

chad1008
Copy link
Contributor

@chad1008 chad1008 commented Jul 4, 2024

Part of 7884-gh-Automattic/dotcom-forge

Proposed Changes

Adds some styles and a few new variables to the Calypso sidebar stylesheet to allow more versatility within individual color schemes to better match wp-admin

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 the first step in making sure our schemes can match what wp-admin displays.

Important Note On Merging:

This PR is a pre-requisite for other PRs for each individual color scheme. I wanted to keep them separate so an individual scheme is easy to revert if needed.

That means this PR has to be merged before any of the others listed in 7884-gh-Automattic/dotcom-forge. It also means, because this PR introduces a few variables that the color schemes on production don't recognize, that we should merge this one and then immediately merge the individual scheme PRs. Ideally, I'd like to merge them all in one go so they can all stack into the same deploy so there's no lag time where specific sidebar elements will be unstyled. The new variables (see below) for the hover state of currently selected submenu items would be the most impactful one if there was a delay on a given scheme.

New variables:

  • --color-sidebar-submenu-selected-hover-text: On some schemes, the currently selected submenu item (e.g. Posts > All Posts) does not change its color when hovered. On other schemes, the hover state matches that of unselected submenu items and the color does change. This variable allows us to make that determination on a per-scheme basis.
  • --color-navredesign-sidebar-submenu-selected-hover-text: This variable accomplishes the same thing as the previous one, but for the Nav Redesign variant of the sidebar, which appears when a site has wp-admin set in interface setting and the My Home/Hosting page is being viewed (this is the only Calypso page that gets rendered when a wp-admin preference is set.
  • --color-collapse-menu-text: Different themes handle the color of this menu differently. Sometimes it matches the base sidebar text color, other times the submenu hover color, or maybe just a separate color all together. The hover state seems to be consistent in that it matches the submenu hover color across all schemes, so we thankfully don't need additional specificity there.

Testing Instructions

This PR alone won't fully address most schemes, so I'd recommend visual code review to confirm the new variables and changes to existing styles all make sense. When reviewing the schemes themselves the styles will all come together, making a review of the UI more practical.

The individual scheme PRs are all branched off of this one, and can be rebased if we make any changes here to keep them current.

@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug chad1008/sidebar-style-updates on your sandbox.

@matticbot
Copy link
Contributor

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.

Copy link
Contributor

@nightnei nightnei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@chad1008

Ideally, I'd like to merge them all in one go so they can all stack into the same deploy so there's no lag time where specific sidebar elements will be unstyled.

Hm, in this case I didn't get the point to split them into different PRs 😅

@chad1008
Copy link
Contributor Author

chad1008 commented Jul 6, 2024

Thanks, Vova!

Hm, in this case I didn't get the point to split them into different PRs 😅

Yeah, I went back and forth on it. Ultimately I wanted to make sure an individual scheme was easy to revert indivdually if needed. I don't think that's likely, but better safe than sorry. I also thought testing one massive PR across two different Calypso sidebars and 15ish schemes would be a nightmare. 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants