Remove more colors #23648
Remove more colors #23648
Conversation
jasmussen
requested review from
ajitbohra,
ajlende,
chrisvanpatten,
ellatrix,
etoledom,
mkevins,
pinarol,
SergioEstevao,
Soean,
talldan and
youknowriad
as code owners
|
Size Change: -288 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
I’ll keep this running locally today just to see if I catch anything weird, but a quick glance says all is well. I’m really excited to have less colors to choose from — I never know when to use what, and having a more focused palette makes me happy. Nice work as usual. |
| $gray-600: #949494; // Meets 3:1 UI or large text contrast against white. | ||
| $gray-400: #ccc; | ||
| $gray-200: #ddd; // Used for most borders. | ||
| $gray-100: #f0f0f0; |
There was a problem hiding this comment.
It's great too see less colors. Maybe later we could try to name these variables differently to clarify their purpose.
for example instead of $gray-200 it could $border-color etc...?
There was a problem hiding this comment.
I would vote for keeping the names generic. A certain color may be recommended for borders, but will likely be used in other contexts as well. Reading css can get a little confusing with explicit var names.
I like having the descriptions / recommendations in this file.
There was a problem hiding this comment.
This for me reduces the value of the variables. Why $black instead of black?
I understand that variables can be used for different things and in that case for me we need two variables that may use the same color. That way when we change a color, we know the impact while right now if you change say the value of $gray-100, you have no idea what you're doing.
There was a problem hiding this comment.
in that case for me we need two variables that may use the same color
I didn't think about that - good point.
There was a problem hiding this comment.
Why $black instead of black?
Because you might want a $black that's actually #000009 to have just a smidgeon of blue in it. This is less of a case now that we're actually moving towards pure neutral grays, but the old grays had a slight cold tinge. I'm happy to remove black and white variables in favor of the CSS colors instead.
More importantly, the fact that it's registered in the color variables sheet, hopefully, trains a developer to look there for guidance before using any colors at all. In this case, you should not use pure black unless you know what you're doing. The primary use case is to create drop shadows, in which case it's appropriate. But given the gray-900 is so very close to black, people might look at the block UI and think "oh it has a black border, I'll just use black". They might still, but it's harder to search the codebase for black than it is to search for $black, and correct any errors.
That way when we change a color, we know the impact while right now if you change say the value of $gray-100, you have no idea what you're doing.
I hear this, and I think this topic is actually the most important to work out in this PR. I intentionally moved from $dark-gray-primary to $gray-900, because I felt the former was fairly confusingly named. In fact I created this gray scale to figure out where to map the grays, on a scale from 0 to 10, 0 being white, 10 being black:
The swatches that are pushed downwards do not exist yet as variables. And maybe we don't want them to? The intent more than anything is fewer colors, and I definitely hear your point, because you really should have some guidance on which colors to use for which UI elements — this is for block UI after all.
Rewinding a bit, here's before:
$black: #000; // Use only when you truly need pure black. For UI, use $dark-gray-primary.
$dark-gray-primary: #1e1e1e;
$medium-gray-text: #757575; // Meets 4.6:1 text contrast against white.
$light-gray-ui: #949494; // Meets 3:1 UI or large text contrast against white.
$light-gray-secondary: #ccc;
$light-gray-tertiary: #e7e8e9;
$white: #fff;
Here's after:
$black: #000; // Use only when you truly need pure black. For UI, use $gray-900.
$gray-900: #1e1e1e;
$gray-700: #757575; // Meets 4.6:1 text contrast against white.
$gray-600: #949494; // Meets 3:1 UI or large text contrast against white.
$gray-400: #ccc;
$gray-200: #ddd; // Used for most borders.
$gray-100: #f0f0f0;
$white: #fff;
I like the systematizing of these. Could we do something like this?
$black-pure: #000;
$gray-900-block-ui: #1e1e1e;
$gray-700-aa-contrast: #757575;
$gray-600-aa-ui-contrast: #949494;
$gray-400: #ccc;
$gray-200-borders: #ddd;
$gray-100-light-borders: #f0f0f0;
$white-pure: #fff;
Or maybe that's too verbose, we could also ditch the white and black as Riad implies, and reduce to this:
$gray-900-block-ui: #1e1e1e;
$gray-700-aa: #757575;
$gray-600-aa-ui: #949494;
$gray-400: #ccc;
$gray-200-borders: #ddd;
$gray-100-borders: #f0f0f0;
Let me know your thoughts.
There was a problem hiding this comment.
I think these latest variables are better, that said when you name a constant when you're coding, you don't name it something like: MAX_COLUMNS_8 you name it just MAX_COLUMNS (the value is not on the name).
I do see where you're coming from, defining a limited set of colors to be used in the UI, so I think the solution might be to use "two sets of variables". One that is internal to the _variables file and should never be used elsewhere, this:
$black: #000; // Use only when you truly need pure black. For UI, use $gray-900.
$gray-900: #1e1e1e;
$gray-700: #757575; // Meets 4.6:1 text contrast against white.
$gray-600: #949494; // Meets 3:1 UI or large text contrast against white.
$gray-400: #ccc;
$gray-200: #ddd; // Used for most borders.
$gray-100: #f0f0f0;
$white: #fff;
And use this set to define the actual variables used in the different modules
$block-ui-border-color: $gray-900;
$dark-border-color: $gray-200;
$light-border-color: $gray-100;
I don't like the dark/light too much because it's not really clear when to use one or the other but it's a bit better. I also intentionally removed the others you defined above to "force us" to add more variables to define where they are being used as right now it seems we don't really know, it's kind of random.
There was a problem hiding this comment.
To be clear, I'm happy to do revert to having just the verbosely named colors, instead of the scale-numbered ones. If need be, I can write the scale value in an HTML comment on the side.
There was a problem hiding this comment.
This feedback is non-blocking as this PR is better than master :)
There was a problem hiding this comment.
Okay, great feedback all around.
There are still 12 more colors that I need to retire and replace. But those won't make it for 5.5. But this PR might. So for the time being, I'm going to keep the numbered grays, and I will follow up with two more PRs (both which won't make 5.5). The first followup will retire the remaining grays, the 2nd followup will open a discussion as to how to best name these colors, for example per Riad's thoughts in #23648 (comment). Sound good?
There was a problem hiding this comment.
Another possibility is to have define some colours and then create aliases. So $border-color = $gray-200.
Thank you Shaun. It's high time we cleaned this up, and I'm so happy to see that in doing so, it also helps tighten the UI up. |
jasmussen
force-pushed
the
remove/more-colors-variables
branch
from
94d4231 to
da1b926
Compare
|
Per conversation in #23648 (comment), it seems this PR is actually ready to go. That means, please continue to test, and get a sanity check for it. And if it's still feeling good, please approve so that we can merge before sunday and make it for 5.5. Thanks all! |
|
Let's merge and test in master. |
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 is a big one, that touches a lot of files. More than anything, it needs a good testing.
The purpose of this PR, as it was with the last one (#23454) is to reduce the total number of colors registered in the global variables sheet. In doing so, we encourage consistent use of a more controlled set of colors, ultimately resulting in a more consistent interface.
Screenshots:
(the text inside cover with a dark background is now white by default, vs. light gray)
In most cases, you shouldn't see a big difference. Nothing should jump out at you, at least. The improvements to visuals is a positive side-effect, but the primary intention with this cleanup is to reduce the surface area of our grays, from 40+ to 8 or 10.