-
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
Nav Unification: introduce Classic Dark as the new default color scheme #47151
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~48 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 (~95 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. |
CC'ing all designers involved for design review @sfougnier @lcollette @davemart-in @rickybanister Given that this would replace the default color scheme for everyone that doesn't have color scheme set, I'd highly appreciate if y'all could take a look and let me know about any changes we'd want to make. Thank you! |
--color-masterbar-item-hover-background: var( --studio-blue-70 ); | ||
--color-masterbar-item-active-background: var( --studio-blue-90 ); | ||
--color-masterbar-item-hover-background: #333; | ||
--color-masterbar-item-active-background: #23282d; |
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.
@sfougnier @lcollette
These two colors (#333, #23282d) are the exact values from wp-admin. If I understand the new Navigation Unification, Design Update (see p2-pbAPfg-13V) correctly, it seems like for brand consistency we're starting to tweak wp-admin colors a bit too. If so could you let me if these should be replaced with color studio vars and if so which ones? Thank you!
If possible, I'd very much prefer to make this swap for everyone who signs up past the timestamp for when we launch this. Otherwise this is going to change the color scheme for hundreds of thousands of customers, perhaps millions with no warning or explanation as to why it happened. I know introducing a signup date stamp will introduce complexity on the code side of things, but this is the only real option that I see. |
Next steps as I understand them from the meeting:
|
It seems to me that Classic Dark will be the default for all users with no specific scheme selected after nav unification is launched. Till then we can launch it but not make it default. I am not sure we should introduce any signup date timestamp. |
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.
Thanks @frontdevde for wrangling this!
Followed the testing instructions and LGTM!
- Booting calypso with a user with no color scheme selected, defaulted to Classic Dark.
- Navigated around and everything seems good.
- Changing to Classic Bright or Classic Blue works as expected
Following this comment #47151 (comment), I would suggest making Classic Bright the default again and creating a separate file for Classic Dark. When we launch nav unification to 100% of users we can move the Classic Dark styles to the default and remove the separate Classic Dark file.
Closing this PR as the approach changed. Will open a new PR to address #47266. |
Changes proposed in this Pull Request
This PR
Context:
With the nav unification project we're introducing unified default colors across Calypso and wp-admin. @rickybanister suggested we introduce these colors to Calypso users early via a new default color scheme. The colors in this PR are based on the most recent Navigation Unification, Design Update (see p2-pbAPfg-13V).
Related issue: #47135
Testing instructions
yarn && yarn start
, visit http://calypso.localhost:3000/me/accountFixes #47135