Retire numerous colors from the _colors.scss #23454
Conversation
jasmussen
added
[Type] Enhancement
jasmussen
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
Soean,
talldan and
youknowriad
as code owners
|
Worth noting, I want to follow this one up with additional PRs that retire the remaining colors, plus another PR that adds a choice few colors. I CC'ed you, @iamthomasbishop, because in working on this I did not touch any of the native stuff. I'm happy to touch that also, but wanted your thoughts first. |
|
Size Change: +44 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
| @@ -12,7 +12,8 @@ $dot-scale: 3; // How much the pulse animation should scale up by in size | |||
|
|
|||
| &::before { | |||
| animation: nux-pulse 1.6s infinite cubic-bezier(0.17, 0.67, 0.92, 0.62); | |||
| background: rgba($blue-medium-800, 0.9); | |||
| background: rgba(#00739c, 0.9); | |||
There was a problem hiding this comment.
I wonder if this should use the theme colors (css vars)
There was a problem hiding this comment.
It should. But I don't know where we use these anymore, so I wasn't able to test this one, and it looks as if the rgba color opacity is animated, which we can't easily do with the CSS variables — right? So I left it alone for now.
There was a problem hiding this comment.
it's a deprecated component, so not very important for sure.
|
I would love a bunch of eyes on this one. I consider it pretty safe because it only changes a few colors, and in almost all cases it will simply result in simplicity and better contrast. But in case there's an obscure place I missed a few beats, would love testing. |
|
Awesome @jasmussen ! I'm gonna give this a spin 🤞 |
|
Thank you! |
| @@ -46,7 +46,7 @@ | |||
| } | |||
|
|
|||
| &:hover { | |||
| color: $blue-medium-400; | |||
| color: var(--wp-admin-theme-color); | |||
| } | |||
There was a problem hiding this comment.
Hmm, this one probably needs to be lighter, or just be removed. (Note to self)
| @@ -12,7 +12,8 @@ $dot-scale: 3; // How much the pulse animation should scale up by in size | |||
|
|
|||
| &::before { | |||
| animation: nux-pulse 1.6s infinite cubic-bezier(0.17, 0.67, 0.92, 0.62); | |||
| background: rgba($blue-medium-800, 0.9); | |||
| background: rgba(#00739c, 0.9); | |||
| opacity: 0.9; | |||
There was a problem hiding this comment.
This line shouldn't have been committed. I'll remove it.
There was a problem hiding this comment.
@jasmussen I tested it a bunch in several browsers. Things look okay to me! There aren't any issues with build.
My only thought is that (maybe) 3rd party consumers of @wordpress/base-styles. We'd probably have to minor bump that would with migration info in changelogs.
Other than that, it's 🚀 from me!
|
Awesome, thanks so much for testing, Q.
I can put together a dev note for sure. But can you clarify this a little bit — wouldn't you have to be doing a full on fork in order for this to be an upgrade blocker? I.e. since these are SCSS variables, they aren't really surfaced in a meaningful way. |
No, this doesn't require a dev note more a "Breaking change" section on the CHANGELOG.md of the package because it's a breaking change for npm users, not for WordPress users. |
I'll definitely help write that, but just for my own understanding, what makes this change breaking for NPM users compared to any other of the many SCSS change we make every day? |
|
@jasmussen we're probably doing similar breaking changes inadvertently. Removing a Sass variable is a breaking change for npm users as someone using the package can't ensure his own styles will continue to work after the update if he's using these variables. |
|
Oh of course, thank you. I'd like to retire more, and do some renaming. But I meant to do that in a follow-up to not make the PR too big. But maybe I should do it all in one? |
|
It's fine to do multiple breaking changes for npm packages. (unlike WP). so just do separate PRs as usually, and if the version of the package is not out, you can just continue adding list items to the breaking changes in the CHANGELOG.md |
|
🌟 This PR makes me so happy — we really do not need all those extra variables. The additional restraint here will help us all be more consistent in designing new pieces in Gutenberg, so I'm all for it. I've taken this for a brief spin and haven't noticed anything that appears incorrect. 👍 |
Thanks for the ping, @jasmussen! Short version: assuming there isn't anything here that breaks mobile, you can feel free to ignore mobile-specific stuff. Long version: we will be revamping our color usage approach in the near future to make use of PlatformColor in RN with a fall-back to a cross-platform "neutral" system that feels natural on both platforms. // ping @maxme let's keep this in mind when we tackle the colors work in the near future. |
jasmussen
force-pushed
the
try/retire-colors
branch
from
92c245e to
1902b50
Compare
|
@youknowriad tests are passing and I'd love to merge, can you glance at my changelog update to see that I did it right? |
|
you did right since the other change is also marked as "unreleased" but in reality the other change is already released so I wonder why the changelog is not up to date. We probably missed a backport to master when we released the latest npm packages cc @gziolo Feel free to merge this PR though. |
|
It should trigger the major version bump next time we do npm publishing. We have a very naive way of detecting version changes and it didn't match the header with higher importance than it should be. See the commit with a fix: |
Following these changes: - WordPress/gutenberg#23454 - WordPress/gutenberg#23648 - WordPress/gutenberg#23048
Following these changes: - WordPress/gutenberg#23454 - WordPress/gutenberg#23648 - WordPress/gutenberg#23048
Following these changes: - WordPress/gutenberg#23454 - WordPress/gutenberg#23648 - WordPress/gutenberg#23048
This PR retires numerous colors from our color variables stylesheet.
Why? Because when we reduce the amount of available colors, it adds focus, clarity, simplicity, and helps tighten up the visual expression of the editor. You really shouldn't need 10 shades of gray, or even pick more than a few.
This also benefits the recently merged theme CSS variables for the spot color, and it benefits any future efforts the core project makes around the admin colors chemes.
There should be very few meaningful visual changes when you test this, perhaps the only ones that might jump out are:
This placeholder text has slightly more contrast.
The dropzone is now opaque, and deliciously theme colored: