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.

Sign up for GitHub

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

Components: Lighten borders to gray-600 #46252

Merged
merged 6 commits into from
Dec 6, 2022
Merged

Components: Lighten borders to gray-600 #46252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3d035e1
Remove unneeded CONFIG variable
mirka Dec 1, 2022
50ae0be
Update `input` mixin border to gray-600
mirka Dec 1, 2022
c66de5d
Add semantic sass var
mirka Dec 1, 2022
f8aabf8
Update colors-values.js
mirka Dec 1, 2022
6771825
Update snapshots
mirka Dec 2, 2022
73488e6
Add changelog
mirka Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/base-styles/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Enhancements

- Lighten the border color in the `input-style__neutral` mixin ([#46252](https://github.com/WordPress/gutenberg/pull/46252)).

## 4.13.0 (2022-11-16)

## 4.12.0 (2022-11-02)
Expand Down
2 changes: 1 addition & 1 deletion packages/base-styles/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@
box-shadow: 0 0 0 transparent;
transition: box-shadow 0.1s linear;
border-radius: $radius-block-ui;
border: $border-width solid $gray-700;
border: $border-width solid $gray-600;
@include reduce-motion("transition");
}

Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
- `ResizableBox`: Prevent unnecessary paint on resize handles ([#46196](https://github.com/WordPress/gutenberg/pull/46196)).
- `Popover`: Prevent unnecessary paint caused by using outline ([#46201](https://github.com/WordPress/gutenberg/pull/46201)).
- `PaletteEdit`: Global styles: add onChange actions to color palette items [#45681](https://github.com/WordPress/gutenberg/pull/45681).
- Lighten the border color on control components ([#46252](https://github.com/WordPress/gutenberg/pull/46252)).

### Experimental

Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/base-field/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the CONFIG.controlBorderColor* variables and consolidate to COLORS.ui. (It was only used in this one instance.)

box-shadow: ${ CONFIG.controlBoxShadow };
display: flex;
flex: 1;
Expand All @@ -30,7 +30,7 @@ export const BaseField = css`
}

&:hover {
border-color: ${ CONFIG.controlBorderColorHover };
border-color: ${ COLORS.ui.borderHover };
}

&:focus,
Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/base-field/test/__snapshots__/index.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Snapshot Diff:
"background": "#fff",
"border": "1px solid",
- "border-color": "#d94f4f",
+ "border-color": "#757575",
+ "border-color": "#949494",
"border-radius": "2px",
"box-shadow": "transparent",
"display": "flex",
Expand All @@ -28,7 +28,7 @@ Snapshot Diff:
@@ -14,19 +14,18 @@
"background": "#fff",
"border": "1px solid",
"border-color": "#757575",
"border-color": "#949494",
"border-radius": "2px",
"box-shadow": "transparent",
- "display": "inline-flex",
Expand Down Expand Up @@ -62,7 +62,7 @@ Snapshot Diff:
"background": "#fff",
- "background-color": "transparent",
"border": "1px solid",
"border-color": "#757575",
"border-color": "#949494",
"border-radius": "2px",
"box-shadow": "transparent",
"display": "flex",
Expand All @@ -89,7 +89,7 @@ exports[`base field should render correctly 1`] = `
background: #fff;
border-radius: 2px;
border: 1px solid;
border-color: #757575;
border-color: #949494;
box-shadow: transparent;
display: -webkit-box;
display: -webkit-flex;
Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/dimension-control/test/__snapshots__/index.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ exports[`DimensionControl rendering renders with custom sizes 1`] = `
position: absolute;
right: 0;
top: 0;
border-color: #757575;
border-color: #949494;
border-style: solid;
border-width: 1px;
padding-left: 2px;
Expand Down Expand Up @@ -445,7 +445,7 @@ exports[`DimensionControl rendering renders with defaults 1`] = `
position: absolute;
right: 0;
top: 0;
border-color: #757575;
border-color: #949494;
border-style: solid;
border-width: 1px;
padding-left: 2px;
Expand Down Expand Up @@ -726,7 +726,7 @@ exports[`DimensionControl rendering renders with icon and custom icon label 1`]
position: absolute;
right: 0;
top: 0;
border-color: #757575;
border-color: #949494;
border-style: solid;
border-width: 1px;
padding-left: 2px;
Expand Down Expand Up @@ -1019,7 +1019,7 @@ exports[`DimensionControl rendering renders with icon and default icon label 1`]
position: absolute;
right: 0;
top: 0;
border-color: #757575;
border-color: #949494;
border-style: solid;
border-width: 1px;
padding-left: 2px;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/form-token-field/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@
transition: all 0.15s ease-in-out;
@include reduce-motion("transition");
list-style: none;
border-top: $border-width solid $gray-700;
border-top: $border-width solid $components-color-border;
margin: 0;
padding: 0;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/utils/colors-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const UI = {
themeDark10: ADMIN.themeDark10,
background: white,
backgroundDisabled: GRAY[ 100 ],
border: GRAY[ 700 ],
border: GRAY[ 600 ],
borderHover: GRAY[ 700 ],
borderFocus: ADMIN.themeDark10,
borderDisabled: GRAY[ 400 ],
Expand Down
2 changes: 0 additions & 2 deletions packages/components/src/utils/config-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ const CONTROL_PROPS = {
controlPaddingXSmall: `calc(${ CONTROL_PADDING_X } / 1.3334)`,
controlBackgroundColor: COLORS.white,
controlBorderRadius: '2px',
controlBorderColor: COLORS.gray[ 700 ],
controlBoxShadow: 'transparent',
controlBorderColorHover: COLORS.gray[ 700 ],
controlBoxShadowFocus: `0 0 0 0.5px ${ COLORS.ui.theme }`,
controlDestructiveBorderColor: COLORS.alert.red,
controlHeight: CONTROL_HEIGHT,
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/utils/theme-variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will eventually make this Sass file the source of truth, and make the colors-values.js file read exported values from here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interested in how is the JS code able to read from SCSS ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interested in how is the JS code able to read from SCSS ?

sass-loader can do this ✨

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the details!