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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Draft

Calypso Color Schemes: Scope the accent variable for GB components. #99159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/calypso-color-schemes/src/shared/color-schemes/_default.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* Default color scheme for the WP.com Host interface.
* Note, this is different from the default wp-admin color scheme, which is either "Default" (_fresh.scss) or "Modern" (_modern.scss).
*/

:root {
/* Theme Properties */
--color-primary: var(--studio-blue-50);
Expand Down Expand Up @@ -363,6 +368,6 @@
--color-navredesign-sidebar-submenu-hover-text: #72aee6; /* Direct from wp-admin */

/* Gutenberg components */
--wp-admin-theme-color: var(--color-accent);
--wp-admin-theme-color-darker-20: var(--color-accent-60);
Comment on lines -366 to -367
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

The --wp-admin-theme-color var used sparingly across Calypso and I've confirmed in a few places (like the verbum comment block editor) that colors still work as expected with a few color schemes (including Modern and Default):

Screenshot 2025-01-31 at 14 25 20

Copy link
Member Author

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!

Copy link
Member

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.

// We intentionally do not style --wp-admin-theme-color and --wp-admin-theme-color-darker-20 at the :root scope
// to avoid conflicts with the default wp-admin color scheme.
}
5 changes: 5 additions & 0 deletions packages/calypso-color-schemes/src/shared/color-schemes/_fresh.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* Classic wp-admin color scheme, aka "Fresh".
* This is default in WordPress core. Modern is set as default for new Simple sites.
*/

.color-scheme.is-fresh {
--color-primary: #2271b1;
--color-accent: var(--color-primary);
Expand Down
Loading