Components: Try color theming #44668
Merged
Conversation
This file contains hidden or 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: +235 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
23 tasks
ciampo
force-pushed
the
try-component-theme
branch
2 times, most recently
from
848aa19 to
653dca3
Compare
ciampo
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar and
ryanwelcher
as code owners
ciampo
changed the title
ciampo
reviewed
Oct 5, 2022
ciampo
force-pushed
the
try-component-theme
branch
from
5f5984f to
ce15b70
Compare
mirka
commented
Oct 11, 2022
|
|
||
| <Theme { ...args }> | ||
| <Button variant="primary"> | ||
| Inner theme (set via Storybook controls) |
| import type { ReactNode, CSSProperties } from 'react'; | ||
|
|
||
| export type ThemeProps = { | ||
| accent?: CSSProperties[ 'color' ]; |
Member
Author
There was a problem hiding this comment.
We might want to document that some otherwise "valid" values cannot be parsed in this context, like currentColor or var(--some-css-var).
Doesn't have to be in this PR, but we can also check at runtime with .isValid() and throw a dev warning.
Contributor
There was a problem hiding this comment.
Added a note in the README and validation via the isValid function (including unit tests) in 0ac7a24
A few additional notes:
- writing tests for
isValidactually flagged a bug in the code: named colors were supported in Storybook because other components also added the needed plugin, but theThemecomponent didn't do it and therefore unit tests were failing! I fixed by adding thenamedplugin explicitly in theThemecomponent too - I've also added the
a11yplugin while I was at it, since we will need it for contrast checking (it's already used in other components, so it shouldn't really make a difference in terms of bundle size) - I changed the type to
string, since theCSSProperties[ 'color' ]was including a bunch of values that we don't support. But let me know if you'd rather revert toCSSProperties[ 'color' ]!
Let me know if these changes LGTY and I will proceed with merging :)
Member
Author
There was a problem hiding this comment.
writing tests for
isValidactually flagged a bug in the code
Wonderful 😄
Looks great! I think we're good to merge. Maybe copy & paste the readme stuff into JSDoc so we get it in Storybook and IntelliSense.
Contributor
There was a problem hiding this comment.
Cool, it turns out that the TS docs already had the README stuff (including Storybook and intellisense).
I'm going to merge once CI is ✅
Member
Author
Smoke testing in the editor is looking ok 🚢 |
Contributor
|
Exciting! How close are we to using components on a dark background now? |
Contributor
|
Hey @jasmussen , this was only the very first step of the process - we still have quite a few aspects to work on, but this PR was definitely a big step in the right direction! We have a tentative TODO list in #44116 (comment) |
Contributor
|
Thanks for tackling! |
14 tasks
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.
What?
Themecomponentaccentcolor exposed as a theme variablecolordlibraryButtoncomponentWhy?
This part of the initial exploration for allowing the
@wordpress/componentsto be themeable (see #44116)Testing Instructions
Themecomponent in Storybook:Buttonin the "Default" example still has the same color as theButtonin theButtonstoryButtons in the "Nested" example should be blue (outer) and orange (inner)Buttonelement renders like ontrunkin the editor and in its dedicated Storybook examplesScreenshots or screencast