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

Update color-studio to 4.1.0 #99033

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

Conversation

matt-west
Copy link
Contributor

@matt-west matt-west commented Jan 29, 2025

Proposed Changes

Why are these changes being made?

The legacy blue is not part of the brand colors, and shouldn't be used for branded elements within Calypso.

Testing Instructions

We will need to extensively test all areas where $studio-blue variables are used in the UI to ensure this change doesn’t have unintended consequences for aesthetics or accessibility.

We should also check instances where $studio-woocommerce-purple is used as this palette also changed in a previous release that was not adopted in Calypso yet.

  • Checkout this branch.
  • Run yarn to install the update version of color-studio.
  • Run yarn start to start Calypso locally
  • Navigate to http://calypso.localhost:3000
  • Check that assets compile correctly
  • Check instances where $studio-blue and $studio-woocommerce-purple variables are used to ensure proper contrast and aesthetics are maintained

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@matt-west matt-west requested a review from jasmussen January 29, 2025 11:51
@matt-west matt-west self-assigned this Jan 29, 2025
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 29, 2025
@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

  • blaze-dashboard
  • command-palette-wp-admin
  • happy-blocks
  • help-center
  • notifications
  • odyssey-stats
  • whats-new
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/color-studio-4.1.0 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.

@matt-west matt-west changed the title Updated color-studio to 4.1.0 Update color-studio to 4.1.0 Jan 29, 2025
@jasmussen
Copy link
Member

Nice one. Code changes look good, and the overall motivation for this PR is also good: the old blue is not part of the brand guidelines and does not make sense to maintain or keep around.

There's also a neat side-effect of code quality to this; instead of propogating the codebase with a mix of studio-blue and studio-wordpress-blue, there's just one blue, and it's the right one. Already, the focus styles here benefit from this:

Screenshot 2025-01-29 at 13 01 33

Those still need work to be unified into a single style, but the point remains, at least now the color is coherent with the accent color, intrinsically. On that, as well as all the spot-testing I could do in testing this PR, ✅ from a design perspective, happy to approve, this is a substantial and good step forward.

That said, this touches a lot of moving pieces, we want to make sure this is thoroughly tested. Who do we need to give heads-up about the change? CC: @Automattic/dotcom-design, and @tyxla, in case they have instincts.

Separately, this also underscores the need to continue with PRs like #98732 (which I'll rebase and shepherd on when this one lands), whose purpose it is to replace instances of hardcoded colors with the studio variables. Just one example from spot-testing, this TOC has a mix of old and new:

Screenshot 2025-01-29 at 13 00 43

Note, the above screenshot is the same before and after this PR, and isn't affected. That's just to say, there's a lot of followup work to also do, which I'll try to stay close to.

@tyxla
Copy link
Member

tyxla commented Jan 29, 2025

That said, this touches a lot of moving pieces, we want to make sure this is thoroughly tested. Who do we need to give heads-up about the change? CC: @Automattic/dotcom-design, and @tyxla, in case they have instincts.

I agree it touches a lot. Just to confirm, this is a comprehensive list of all the changes:

https://npmdiff.dev/%40automattic%2Fcolor-studio/3.0.1/4.1.0/

I'd ping at least one designer from each product that's shipped in Calypso, to review and test some of the flows, since this literally affects all the flows around WP.com, Jetpack Cloud and A4A.

If color changes are subtle, it's likely OK, but still, worth double-checking since there are too many color changes too 😅

@matt-west
Copy link
Contributor Author

Just to confirm, this is a comprehensive list of all the changes:

Yes, that's correct. Worth noting that this also includes updates to WooCommerce Purple which weren't reflected in Calypso yet as it was still running an older version of color-studio.

@jasmussen
Copy link
Member

I'm diving into something tangential to this, and finding out that there will need to be some color scheme updates of, at least, this admin color scheme: https://github.com/Automattic/wp-calypso/tree/trunk/packages/calypso-color-schemes

"Classic Blue", specifically, whose counterpart, best I can tell, is "Default" in wp-admin contexts. Those colors don't exist anymore. Not sure how broadly that particular color scheme is used, but if it goes beyond just wp-admin and into calypso, it suggests that we might need to keep the old colors around after all, perhaps "studio-blue-classic" or the like.

The alternative, if it's only wp-admin, is to update https://github.com/Automattic/wp-calypso/blob/trunk/packages/calypso-color-schemes/src/shared/color-schemes/_classic-blue.scss with hardcoded color values.

@jasmussen
Copy link
Member

Further context from #99159 (comment), we need to fully grok the calypso color scheme system, and which files (between _default, _fresh, and _modern), actually make up the defaults and where. My instinct at this point is that _fresh is the classic WordPress core color set, which is set by default on Atomic (Modern is set on Simple), and that the fix here is to replace all the variables in _fresh.css with hard-coded values directly from the admin schemes. If someone could help affirm these instincts, I'd be happy to push code to help make that happen in this PR.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants