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 hardcoded blue color values to match Studio counterparts #98732

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

Conversation

jasmussen
Copy link
Member

Related to https://github.com/Automattic/dotcom-forge/issues/10307#issuecomment-2607061431, alternative to #98730

Proposed Changes

This PR scoured the codebase looking for instances of hard-coded legacy blue colors, and updates them to Blue 50 from Color Studio.

This work will later benefit from Automattic/color-studio#764, which updates the values of Blue 50 to match Blueberry.

Why are these changes being made?

Blueberry is the official WordPress.com accent color, the legacy blue is deprecated.

Testing Instructions

Test the contexts referenced from the list of changed files. That includes the logged out Reader, available at wordpress.com/read in an incognito window. It also includes the cookie notice, and an onboarding stepper.

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)?

@jasmussen jasmussen requested review from a team as code owners January 22, 2025 12:15
@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 22, 2025
@jasmussen jasmussen self-assigned this Jan 22, 2025
@jasmussen jasmussen added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Jan 22, 2025
@jasmussen
Copy link
Member Author

@Automattic/dotcom-design—let me know if this ping is becoming too busy!

@matticbot
Copy link
Contributor

matticbot commented Jan 22, 2025

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~16 bytes added 📈 [gzipped])

name                             parsed_size           gzip_size
plugins                                +32 B  (+0.0%)       -3 B  (-0.0%)
jetpack-cloud-plugin-management        +32 B  (+0.0%)       -3 B  (-0.0%)
a8c-for-agencies-plugins               +32 B  (+0.0%)       -3 B  (-0.0%)
import-hosted-site-flow                +27 B  (+0.0%)      +19 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~3 bytes removed 📉 [gzipped])

name                                     parsed_size           gzip_size
async-load-signup-steps-website-content        +32 B  (+0.0%)       -3 B  (-0.0%)
async-load-design                              +13 B  (+0.0%)       +0 B

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@crisbusquets
Copy link
Contributor

let me know if this ping is becoming too busy!

It's ok for me, not sure about the others 👀

@jasmussen
Copy link
Member Author

CC: @Automattic/team-calypso in case you have thoughts on this approach!

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

The approach looks good, and I confirm the chosen colors are the closest ones and the difference is negligible 👍

Might make sense to have some before/after comparisons of the affected areas, just so we can be sure there are no unexpected differences.

Left a few questions, I don't think they're blockers.

}
}

button.components-button.is-link {
padding: 20px 0 6px 0;
color: #1d2333;
color: var(--studio-blue-90);
Copy link
Member

Choose a reason for hiding this comment

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

The difference between these 2 colors is quite big:

Screenshot 2025-01-23 at 12 05 36

I wonder if that's intentional and if there's a better one.

Still, I confirm that blue-90 is the closest one.

Copy link
Member Author

Choose a reason for hiding this comment

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

CC: @davemart-in since this touches the logged out reader. But I think it's fine. In this case, open http://calypso.localhost:3000/tag/writing in an incognito window, then press "Like" on a post, you'll see this:

Screenshot 2025-01-23 at 13 22 26

The "Log in" button is now getting the studio color. Which has sufficient contrast with a white background.

Copy link
Contributor

Choose a reason for hiding this comment

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

This observation is potentially unrelated – forgive me if so – but should the 'Log in' link color match others in the software? That seems to be the default blue for all admin themes except Modern:

link

Copy link
Member Author

Choose a reason for hiding this comment

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

Important context: "Modern" is both the default WordPress.com brand color, but also the default color scheme that's set for every new Simple site created. Which means if we land this PR and Automattic/color-studio#764, then new users will see a consistent Blueberry, #3858e9 across all those sections, including the example you show.

In https://github.com/Automattic/dotcom-forge/issues/8519#issuecomment-2593275569 it was discussed what the role of this color scheme should be, the decision (CC: @fditrapani) is that if you set a color scheme, we respect that choice for that particular site. The nuance is: that choice is per-site, and you might have 10 sites in your account, each of them with a different color scheme. So what does that mean for sections like /me, /sites, or /read? For the moment, with the two PRs mentioned in the first paragraph, those will be Blueberry, regardless of your individual site color scheme.

What do you think, and did that answer your question?

$onboarding-accent-blue: #117ac9;
$onboarding-accent-blue-background: #fafcfe;
$onboarding-accent-blue: var(--studio-blue-50);
$onboarding-accent-blue-background: var(--studio-blue-0);
Copy link
Member

Choose a reason for hiding this comment

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

Should we double-check the visual impact of this?

The blue-ish tint is quite more noticeable with blue-0:

Screenshot 2025-01-23 at 12 06 59

Copy link
Member Author

Choose a reason for hiding this comment

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

@ciampo can you confirm? As best I can tell, this is touching the blue shades that were introduced here: #47181

Copy link
Contributor

@ciampo ciampo Jan 28, 2025

Choose a reason for hiding this comment

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

That PR was a while ago! Let me take a look and see if I can remember with any additional context about it

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasmussen the previous colors were indeed introduced during the Focused Launch project, but I think that Marin comments still stands — the new suggested colors produce a worse contrast when used in combination:

Before After
Screenshot 2025-01-28 at 14 51 16 Screenshot 2025-01-28 at 14 53 25

In particular, the --studio-blue-0 variable used as the background color seems quite dark.

Are there any guidelines for using studio colors on backgrounds? Could we alternatively use white as the background?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, thanks for testing.

As of this PR, however, with puPv3-nYp-p2 as a counterpart, the new studio color values will be #fbfcfe for Blue 0, and #3858e9 for Blue 50. Resulting in this:

example

Screenshot 2025-01-29 at 12 17 22

I'm happy to postpone rolling this one out until those variables are updated, if that changes the conversation? I can also just omit those particular instances if need be.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's great! Probably cleanest course of action is to update this project to use the latest color studio version, before resuming work on this PR.

I went ahead and opened a PR to update the color studio version: #99037 (although I'll need your and every other dotcom designer help to properly test it)

Copy link
Member

Choose a reason for hiding this comment

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

Just to close the loop, #99033 is already doing the color studio package update and #99037 was closed in its favor.

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] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants