-
Notifications
You must be signed in to change notification settings - Fork 2k
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 Calypso Colors #35949
Update Calypso Colors #35949
Conversation
Test live: https://calypso.live/?branch=update/colors |
Regarding this bit:
Given these updates’ impact on our app accessibility, I think this branch shouldn’t have to wait for all illustrations to be updated. Maybe we could outsource the SVG color updates to one of our creative technologists? I imagine this isn’t the last time we’ll have to perform those. |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~2056 bytes added 📈 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~1137 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~184905 bytes added 📈 [gzipped])
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 (~43844 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
Just eye-balling signup cc @dwolfe @fditrapani @shaunandrews Currently the signup primary color is
|
@ramonjd Thanks for pointing that out. That’s my omission. The signup should use WordPress Blue for better visual continuity with our landing pages. Already fixed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple minor comments/questions.
Before I can test this you should rebase on top of master
since this wasn't updated since August 20th and a lot of code has changed since. ;-) Pay attention e.g. to changes made in #35793
It was mentioned a few times outside Internets that browser inspector has gotten very slow after this PR was merged. cc at least @ramonjd and I can't remember who else mentioned the same. :-) What was your browser? |
I also feel that btw :) |
I experienced the very same problem while working on this, but I’m not sure yet if this update was the one that introduced it. We actually cut down on the number of custom properties with this PR. |
I think the issue might be that all few hundred color variables are appended to each CSS bundle loaded during Calypso session. That'll cause each new CSS bundle download overwriting previous variables in memory but I suppose all those thousands of variables kept in inspector's memory? Seems like color variables should be loaded just once initially (maybe even inline on page render?) and not on every bundle? cc @sgomes as this is clearly a performance issue. |
Thanks for the ping, @simison! This PR seems to have introduced some performance issues, both on the CSS and the JS side of things. I haven't had a chance to look at this thoroughly yet, as the way it was merged (with a merge commit) makes it very hard to isolate from other changes that got interspersed throughout. Adding it to my backlog. |
Thanks @simison and @sgomes for looking into this. I experimented a couple of times by resetting my local branch to the commit immediately previous to 2fcbd46, reinstalling node modules and rebooting Calypso, then resetting forward to 2fcbd46. Confirmed that this effect is noticeable in Chrome, Chromium and Firefox. It's relatively tolerable if you switch away from the styles pane, that is, you can inspect elements and perform other important tasks in debugger, but inspecting and editing styles is no longer fun. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a big change, thanks for taking that on 🙂
Some issues have appeared in and around Calypso that seem to be related to this changeset that we'll need to work to track down.
If we find ways to keep changesets smaller, it should help with reviews in addition to making it easier to track down the source of issues when they are introduced.
.eslintrc.js | ||
bin | ||
screenshot@2x.png | ||
src/__color-studio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke SASS in the published package. Fixed in #36171.
|
||
* Updated [Color Studio](https://color-studio.blog), the primary dependency, to the most recent version. | ||
* Following the [files](https://github.com/Automattic/color-studio/blob/master/dist/color-properties.css) [generated](https://github.com/Automattic/color-studio/blob/master/dist/color-properties-rgb.css) by Color Studio, updated all properties to use consistent names (including index numbers). | ||
* Phased out all SCSS variables in favor of CSS custom properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this happen? This breaks dependent SASS pipelines. Importing CSS is very different from importing SASS in that it generates output.
I'd speculate that this is the cause of #35949 (comment) and is likely related to a bump in bundle sizes @sgomes has been tracking down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I already mentioned in one of the comments, it was a handful of the remaining SCSS variables that were phased out. Those had relied on SCSS color functions like darken
or lighten
, and that was the reason why the original PR introducing calypso-color-schemes
left them out.
In other words, I completed the switch from one SCSS variables to CSS custom properties that had already been done in 95%.
I'm working on addressing the issues here in #36175 |
I'm seeing a few things in this PR that I'm having trouble grokking and understanding correctly. This PR touches a lot of stuff, and it seems to make a bunch of different types of changes:
Combined with the large number of files changed in this PR, this makes it really hard to reason about and to track down some negative effects it appears to have introduced. This was compounded by the fact that the PR was merged with a merge commit, interspersing it with other unrelated commits, and the branch is now deleted. (This is no longer possible, as the repo will now only accept squash merges) Is it possible to get a bit more clarity and background on what's happening here?
In general, it would likely be best if we were to split large changes like this across multiple PRs that build on each other, and would let reviewers focus on one type of change at a time. |
This PR built on the existing concepts introduced earlier with Calypso color schemes. It didn’t really touch any stylesheets postprocessing. There were a few SCSS variables left in places that relied on SCSS color functions like |
@sgomes Could you elaborate on that? |
Noted. Although, please note some of the issues mentioned above had been introduced before I started working on this. I have a local copy of this branch and can bring it back. What else can I do to help? |
In #35949 we missed the OAuth login screen (only used in the desktop app), so it is currently using a wrong color for its background. This PR uses the proper color var for the background screen, so the WP.com's brand color is used (same color as sign up). It also makes the sign it button enabled by default and removes the custom blue color used on it, so it is displayed with the same pink used in other primary action buttons.
In #35949 we missed the OAuth login screen (only used in the desktop app), so it is currently using a wrong color for its background. This PR uses the proper color var for the background screen, so the WP.com's brand color is used (same color as sign up). It also makes the sign it button enabled by default and removes the custom blue color used on it, so it is displayed with the same pink used in other primary action buttons.
This branch updates all Calypso components and all color schemes with the recently refreshed palette colors.
Notable changes
Color schemes
Color scheme properties have been refactored:
--color-
while generic colors are directly outsourced to Color Studio properties (which are prefixed with--studio-
and can’t be overwritten).Components
Some components had to receive dedicated but non-breaking color treatments or refinements for better overall consistency:
Other
Testing instructions