Remove more colors #23648
Remove more colors #23648
Conversation
Size Change: -288 B (0%) Total Size: 1.13 MB
|
Filename | Size | Change | |
---|---|---|---|
build/a11y/index.js |
1.14 kB | 0 B | |
build/annotations/index.js |
3.62 kB | 0 B | |
build/api-fetch/index.js |
3.4 kB | 0 B | |
build/autop/index.js |
2.82 kB | 0 B | |
build/blob/index.js |
620 B | 0 B | |
build/block-directory/index.js |
7.16 kB | 0 B | |
build/block-editor/index.js |
109 kB | 0 B | |
build/block-library/index.js |
130 kB | 0 B | |
build/block-serialization-default-parser/index.js |
1.88 kB | 0 B | |
build/block-serialization-spec-parser/index.js |
3.1 kB | 0 B | |
build/blocks/index.js |
48.2 kB | 0 B | |
build/components/index.js |
198 kB | 0 B | |
build/compose/index.js |
9.65 kB | 0 B | |
build/core-data/index.js |
11.4 kB | 0 B | |
build/data-controls/index.js |
1.29 kB | 0 B | |
build/data/index.js |
8.44 kB | 0 B | |
build/date/index.js |
5.47 kB | 0 B | |
build/deprecated/index.js |
772 B | 0 B | |
build/dom-ready/index.js |
569 B | 0 B | |
build/dom/index.js |
3.19 kB | 0 B | |
build/edit-navigation/index.js |
9.97 kB | 0 B | |
build/edit-post/index.js |
304 kB | 0 B | |
build/edit-site/index.js |
16.6 kB | 0 B | |
build/edit-widgets/index.js |
9.33 kB | 0 B | |
build/edit-widgets/style-rtl.css |
2.45 kB | 0 B | |
build/editor/editor-styles-rtl.css |
537 B | 0 B | |
build/editor/editor-styles.css |
539 B | 0 B | |
build/editor/index.js |
44.8 kB | 0 B | |
build/element/index.js |
4.65 kB | 0 B | |
build/escape-html/index.js |
733 B | 0 B | |
build/format-library/index.js |
7.73 kB | 0 B | |
build/format-library/style-rtl.css |
547 B | 0 B | |
build/format-library/style.css |
548 B | 0 B | |
build/hooks/index.js |
2.13 kB | 0 B | |
build/html-entities/index.js |
622 B | 0 B | |
build/i18n/index.js |
3.56 kB | 0 B | |
build/is-shallow-equal/index.js |
710 B | 0 B | |
build/keyboard-shortcuts/index.js |
2.51 kB | 0 B | |
build/keycodes/index.js |
1.94 kB | 0 B | |
build/list-reusable-blocks/index.js |
3.12 kB | 0 B | |
build/list-reusable-blocks/style-rtl.css |
476 B | 0 B | |
build/list-reusable-blocks/style.css |
476 B | 0 B | |
build/media-utils/index.js |
5.3 kB | 0 B | |
build/notices/index.js |
1.79 kB | 0 B | |
build/nux/index.js |
3.4 kB | 0 B | |
build/nux/style-rtl.css |
671 B | 0 B | |
build/nux/style.css |
668 B | 0 B | |
build/plugins/index.js |
2.56 kB | 0 B | |
build/primitives/index.js |
1.5 kB | 0 B | |
build/priority-queue/index.js |
788 B | 0 B | |
build/redux-routine/index.js |
2.85 kB | 0 B | |
build/rich-text/index.js |
14 kB | 0 B | |
build/server-side-render/index.js |
2.68 kB | 0 B | |
build/shortcode/index.js |
1.69 kB | 0 B | |
build/token-list/index.js |
1.28 kB | 0 B | |
build/url/index.js |
4.06 kB | 0 B | |
build/viewport/index.js |
1.85 kB | 0 B | |
build/warning/index.js |
1.14 kB | 0 B | |
build/wordcount/index.js |
1.17 kB | 0 B |
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; |
youknowriad
Jul 2, 2020
Contributor
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...?
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...?
jameskoster
Jul 2, 2020
Contributor
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.
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.
youknowriad
Jul 2, 2020
Contributor
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.
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.
jameskoster
Jul 2, 2020
Contributor
in that case for me we need two variables that may use the same color
I didn't think about that - good point.
in that case for me we need two variables that may use the same color
I didn't think about that - good point.
jasmussen
Jul 3, 2020
•
Author
Contributor
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.
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.
youknowriad
Jul 3, 2020
Contributor
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.
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.
jasmussen
Jul 3, 2020
Author
Contributor
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.
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.
youknowriad
Jul 3, 2020
Contributor
This feedback is non-blocking as this PR is better than master :)
This feedback is non-blocking as this PR is better than master :)
jasmussen
Jul 3, 2020
Author
Contributor
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?
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?
ellatrix
Jul 3, 2020
Member
Another possibility is to have define some colours and then create aliases. So $border-color = $gray-200
.
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. |
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
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.