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

Jump to bottom

Components: Expand theming support in COLORS #58097

Merged
merged 9 commits into from
Jan 25, 2024
Merged

Components: Expand theming support in COLORS #58097

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3af68c4
Components: Expand theming support in COLORS
mirka Jan 22, 2024
1076842
Update changelog
mirka Jan 22, 2024
14733dd
Update snapshots
mirka Jan 23, 2024
8b321ab
Fix gray-900 var
mirka Jan 25, 2024
e602750
Completely match theme-variables scss
mirka Jan 25, 2024
f0f2710
Replace `ui.textDark` to `theme.foregroundInverted`
mirka Jan 25, 2024
c9020bd
Replace with color-mix
mirka Jan 25, 2024
4353b34
Merge branch 'trunk' into add/theme-colors-js
mirka Jan 25, 2024
88025c1
Fix changelog placement
mirka Jan 25, 2024
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
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 @@ -18,6 +18,7 @@
### Experimental

- `BoxControl`: Update design ([#56665](https://github.com/WordPress/gutenberg/pull/56665)).
- Expand theming support in the `COLORS` variable object ([#58097](https://github.com/WordPress/gutenberg/pull/58097)).
- `CustomSelect`: adjust `renderSelectedValue` to fix sizing ([#57865](https://github.com/WordPress/gutenberg/pull/57865)).

## 25.15.0 (2024-01-10)
Expand Down
55 changes: 39 additions & 16 deletions packages/components/src/utils/colors-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,34 +36,57 @@ const ADMIN = {
'var(--wp-components-color-accent-darker-10, var(--wp-admin-theme-color-darker-10, #2145e6))',
};

const UI = {
background: white,
backgroundDisabled: GRAY[ 100 ],
border: GRAY[ 600 ],
borderHover: GRAY[ 700 ],
borderFocus: ADMIN.theme,
borderDisabled: GRAY[ 400 ],
textDisabled: GRAY[ 600 ],
textDark: white,

// Matches @wordpress/base-styles
darkGrayPlaceholder: rgba( GRAY[ 900 ], 0.62 ),
lightGrayPlaceholder: rgba( white, 0.65 ),
};

// Should match packages/components/src/utils/theme-variables.scss
const THEME = {
accent: ADMIN.theme,
accentDarker10: ADMIN.themeDark10,
background: `var(--wp-components-color-background, ${ white })`,
foreground: `var(--wp-components-color-foreground, ${ GRAY[ 900 ] })`,
gray: {
900: `var(--wp-components-color-gray-900, ${ GRAY[ 900 ] })`,
ciampo marked this conversation as resolved.
Show resolved Hide resolved
800: `var(--wp-components-color-gray-800, ${ GRAY[ 800 ] })`,
700: `var(--wp-components-color-gray-700, ${ GRAY[ 700 ] })`,
600: `var(--wp-components-color-gray-600, ${ GRAY[ 600 ] })`,
400: `var(--wp-components-color-gray-400, ${ GRAY[ 400 ] })`,
300: `var(--wp-components-color-gray-300, ${ GRAY[ 300 ] })`,
200: `var(--wp-components-color-gray-200, ${ GRAY[ 200 ] })`,
100: `var(--wp-components-color-gray-100, ${ GRAY[ 100 ] })`,
},
};

const UI = {
background: THEME.background,
backgroundDisabled: THEME.gray[ 100 ],
border: THEME.gray[ 600 ],
borderHover: THEME.gray[ 700 ],
borderFocus: THEME.accent,
borderDisabled: THEME.gray[ 400 ],
textDisabled: THEME.gray[ 600 ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a text alias under UI ? Currently components would be forced to use theme.foreground instead.

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'm actually kind of reluctant to add a ton of semantic aliases for the pretty obvious ones like foreground, background, and accent because it adds more overhead when reworking and renaming stuff. I do think the aliases for the grayscales are useful because nobody memorizes them and it's easy to introduce inconsistencies. Does that resonate?

Copy link
Contributor

Choose a reason for hiding this comment

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

My original reasoning behind proposing text was mostly so that folks can continue to use "semantic" color aliases to style their components, instead of mixing them with "raw" theme values. But if we're ok with folks using both COLORS.ui.* and COLORS.theme.*, then we can keep things as-is and only adjust later / if needed, especially if the long term plan is to move theming functionality to a separate package.

textDark: white, // TODO: Make this Theme-ready
ciampo marked this conversation as resolved.
Show resolved Hide resolved

// Matches @wordpress/base-styles
darkGrayPlaceholder: rgba( GRAY[ 900 ], 0.62 ), // TODO: Make this work with CSS vars
lightGrayPlaceholder: rgba( white, 0.65 ), // TODO: Make this work with CSS vars
ciampo marked this conversation as resolved.
Show resolved Hide resolved
};

export const COLORS = Object.freeze( {
/**
* The main gray color object.
*
* @deprecated Use semantic aliases in `COLORS.ui` or theme-ready variables in `COLORS.theme.gray`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This deprecation marking should encourage devs to stop using this while we migrate the existing usages.

*/
gray: GRAY,
gray: GRAY, // TODO: Stop exporting this when everything is migrated to `theme` or `ui`
Copy link
Contributor

@ciampo ciampo Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have a tracking issue for all the planned work that we intend to do around theming in the short term?

Edit: yes, we do! It's #44116. We could probably tidy that issue up, knowing that in the long term the Theme component in @wordpress/components will be removed and replaced by functionality from @wordpress/theme.

white,
alert: ALERT,
/**
* Theme-ready variables with fallbacks.
*
* Prefer semantic aliases in `COLORS.ui` when applicable.
*/
theme: THEME,
/**
* Semantic aliases (prefer these over raw variables when applicable).
*/
ui: UI,
} );

Expand Down
Loading