Skip to content
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

Remove more colors #23648

Merged
merged 11 commits into from Jul 3, 2020
Merged

Remove more colors #23648

merged 11 commits into from Jul 3, 2020

Conversation

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Jul 2, 2020

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:

Screenshot 2020-07-01 at 09 52 31

Screenshot 2020-07-01 at 09 52 34

Screenshot 2020-07-01 at 09 52 56

Screenshot 2020-07-02 at 12 11 16

Screenshot 2020-07-02 at 13 32 41

(the text inside cover with a dark background is now white by default, vs. light gray)

Screenshot 2020-07-02 at 13 36 36

Screenshot 2020-07-02 at 13 41 27

Screenshot 2020-07-02 at 13 46 33

Screenshot 2020-07-02 at 13 55 20

Screenshot 2020-07-02 at 14 35 50

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.

@github-actions
Copy link

@github-actions github-actions bot commented Jul 2, 2020

Size Change: -288 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-directory/style-rtl.css 944 B -8 B (0%)
build/block-directory/style.css 945 B -6 B (0%)
build/block-editor/style-rtl.css 10.7 kB -47 B (0%)
build/block-editor/style.css 10.7 kB -47 B (0%)
build/block-library/editor-rtl.css 7.62 kB -14 B (0%)
build/block-library/editor.css 7.62 kB -16 B (0%)
build/block-library/style-rtl.css 7.78 kB -7 B (0%)
build/block-library/style.css 7.79 kB -5 B (0%)
build/block-library/theme-rtl.css 728 B -2 B (0%)
build/block-library/theme.css 729 B -3 B (0%)
build/components/style-rtl.css 15.8 kB -33 B (0%)
build/components/style.css 15.8 kB -31 B (0%)
build/edit-navigation/style-rtl.css 1.02 kB -1 B
build/edit-navigation/style.css 1.02 kB -1 B
build/edit-post/style-rtl.css 5.57 kB +8 B (0%)
build/edit-post/style.css 5.57 kB +8 B (0%)
build/edit-site/style-rtl.css 3.03 kB +2 B (0%)
build/edit-site/style.css 3.03 kB +2 B (0%)
build/edit-widgets/style.css 2.45 kB -1 B
build/editor/style-rtl.css 3.78 kB -41 B (1%)
build/editor/style.css 3.77 kB -45 B (1%)
ℹ️ View Unchanged
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

compressed-size-action

@shaunandrews
Copy link
Contributor

@shaunandrews shaunandrews commented Jul 2, 2020

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;

This comment has been minimized.

@youknowriad

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...?

This comment has been minimized.

@jameskoster

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.

This comment has been minimized.

@youknowriad

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 comment has been minimized.

@jameskoster

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.

This comment has been minimized.

@jasmussen

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:

Colors

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.

This comment has been minimized.

@youknowriad

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.

This comment has been minimized.

@jasmussen

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.

This comment has been minimized.

@youknowriad

youknowriad Jul 3, 2020
Contributor

This feedback is non-blocking as this PR is better than master :)

This comment has been minimized.

@jasmussen

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?

This comment has been minimized.

@ellatrix

ellatrix Jul 3, 2020
Member

Another possibility is to have define some colours and then create aliases. So $border-color = $gray-200.

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Jul 3, 2020

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.

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 jasmussen force-pushed the remove/more-colors-variables branch from 94d4231 to da1b926 Jul 3, 2020
@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Jul 3, 2020

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!

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Jul 3, 2020

Let's merge and test in master.

@ellatrix ellatrix merged commit e259fd0 into master Jul 3, 2020
13 checks passed
13 checks passed
Check Check
Details
build
Details
Admin - 1
Details
pull-request-automation
Details
test (gutenberg-editor-gallery)
Details
test (gutenberg-editor-gallery)
Details
All
Details
JavaScript
Details
Admin - 2
Details
PHP
Details
Admin - 3
Details
Mobile
Details
Admin - 4
Details
@ellatrix ellatrix deleted the remove/more-colors-variables branch Jul 3, 2020
@github-actions github-actions bot added this to the Gutenberg 8.5 milestone Jul 3, 2020
@jasmussen jasmussen mentioned this pull request Aug 3, 2020
6 of 6 tasks complete
jeherve added a commit to Automattic/jetpack that referenced this pull request Aug 3, 2020
jeherve added a commit to Automattic/jetpack that referenced this pull request Aug 3, 2020
jeherve added a commit to Automattic/jetpack that referenced this pull request Aug 4, 2020
jeherve added a commit to Automattic/jetpack that referenced this pull request Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.