Components: Lighten borders to gray-600 #46252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -5 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
mirka
commented
Dec 2, 2022
| @@ -13,7 +13,7 @@ export const BaseField = css` | |||
| background: ${ CONFIG.controlBackgroundColor }; | |||
| border-radius: ${ CONFIG.controlBorderRadius }; | |||
| border: 1px solid; | |||
| border-color: ${ CONFIG.controlBorderColor }; | |||
| border-color: ${ COLORS.ui.border }; | |||
There was a problem hiding this comment.
Remove the CONFIG.controlBorderColor* variables and consolidate to COLORS.ui. (It was only used in this one instance.)
| @@ -23,3 +23,6 @@ $components-color-gray-400: var(--wp-components-color-gray-400, $gray-400); | |||
| $components-color-gray-600: var(--wp-components-color-gray-600, $gray-600); | |||
| $components-color-gray-700: var(--wp-components-color-gray-700, $gray-700); | |||
| $components-color-gray-800: var(--wp-components-color-gray-800, $gray-800); | |||
|
|
|||
| // Semantic aliases (prefer these over raw gray values when applicable). | |||
| $components-color-border: #{ $components-color-gray-600 }; | |||
There was a problem hiding this comment.
I will eventually make this Sass file the source of truth, and make the colors-values.js file read exported values from here.
There was a problem hiding this comment.
Interested in how is the JS code able to read from SCSS ?
There was a problem hiding this comment.
This is actually my only inquiry, these variable names. The 600 gray is not just for borders, and certainly not all borders. For example it's not to be used for the borders separate the sidebar from the content. From the existing variables:
// WordPress grays.
$black: #000; // Use only when you truly need pure black. For UI, use $gray-900.
$gray-900: #1e1e1e;
$gray-800: #2f2f2f;
$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-300: #ddd; // Used for most borders.
$gray-200: #e0e0e0; // Used sparingly for light borders.
$gray-100: #f0f0f0; // Used for light gray backgrounds.
$white: #fff;
Even in the above cases, there are going to be semantic edgecases. I do think at the moment we use it mainly for borders, but I'm still not sure so directly tying it to the term "border" is actually useful. It's not a strong opinion, to be clear, and it seems like mostly an implementation detail since the $components-color-gray-600 is going to continue to exist, right?
Speaking of, is it worthwhile to update color variables across the codebase, and perhaps retire the _colors.scss file, if theme-variables.scss is going to be the new location?
There was a problem hiding this comment.
Interested in how is the JS code able to read from SCSS ?
sass-loader can do this ✨
There was a problem hiding this comment.
certainly not all borders. For example it's not to be used for the borders separate the sidebar from the content. Even in the above cases, there are going to be semantic edgecases. I do think at the moment we use it mainly for borders, but I'm still not sure so directly tying it to the term "border" is actually useful.
This is good to know, thanks. You're right, $components-color-border is just a convenience variable and gray-600 will continue to exist. I'm still in the process of auditing the codebase, so yes, it could turn out that semantically named variables are not a good idea! Given that base-styles _colors.scss doesn't have any semantic variables, maybe wp-components should lean toward simplicity as well.
Speaking of, is it worthwhile to update color variables across the codebase, and perhaps retire the _colors.scss file, if theme-variables.scss is going to be the new location?
Not yet. Until we finalize the details of the theming system, we'll be experimenting with our new variables exclusively within the wp-components package. It's likely that base-styles _colors.scss will remain the official source of truth for everything.
There was a problem hiding this comment.
Thank you for the details!
ciampo
reviewed
Dec 2, 2022
There was a problem hiding this comment.
This looks good under a coding point of view.
I'll let @jasmussen give the green light in terms of design/art direction, and to point out in case we're missing any components that still require updating
|
Values look good, thank you! Gray 600 meets 3:1 UI contrast against white, and works well as input borders. I left some comments/questions above, but nothing blocking here. Thanks for the PR! |
mirka
force-pushed
the
lighten-borders
branch
from
6d5e7ec to
6757cd5
Compare
mirka
force-pushed
the
lighten-borders
branch
from
6757cd5 to
73488e6
Compare
chad1008
approved these changes
Dec 6, 2022
There was a problem hiding this comment.
This looks good and tests well for me! 🚀 🚢
I think the conversation around semantic vs. simple variable names will be an interesting one as we continue testing things with theming. Regarding borders and @jasmussen's point that not all borders use gray-600, I wonder if something like $components-color-border-default (or similar) might be a good middle ground. That way it's semantic, but it's maybe more clear that this is the default border color in the Components package, but some elements will use different/custom borders the way the sidebar does.
|
To be even clearer: $gray-600 can be used for ui contrast (input borders, arguably icons, elements in other componentry, focus styles), or large text (which isn't entirely clear in terms of font sizes, but larger than most UI font sizes we use). In our use, we happen to have only used it for input borders at the moment, and that might continue to be the case. But the color was chosen to match 3:1 contrast and be available for more contexts than just borders. Nothing that need to block this particular PR, so long as we can rename and refine these in the future 👍 👍 |
23 tasks
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
* Remove unneeded CONFIG variable * Update `input` mixin border to gray-600 * Add semantic sass var * Update colors-values.js * Update snapshots * Add changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #39425
What?
Updates the border color of our control components (e.g. InputControl, SelectControl, etc) so it is a shade lighter (gray-600 instead of 700).
Why?
To reflect the updated design spec.
How?
With the theming work (#44116) in mind, we'll use a semantically named intermediary variable (something that includes the word "border") instead of using the
$gray-600variable directly.Testing Instructions
npm run storybook:devmagentaor something, and the changes will be more obvious in Storybook.Screenshots or screencast