[WB-1853] Button: Refactor theme to support semanticColor tokens #2431
Conversation
🦋 Changeset detectedLatest commit: 1b0bc80 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-buxylhokuj.chromatic.com/ Chromatic results:
|
|
Size Change: +193 B (+0.2%) Total Size: 98.2 kB
ℹ️ View Unchanged
|
| @@ -228,50 +228,4 @@ describe("button with icon", () => { | |||
| expect(icon).toBeInTheDocument(); | |||
| expect(icon).toHaveAttribute("aria-hidden", "true"); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
note: These tests no longer apply as tertiary buttons don't need any changes for the Khanmigo theme.
| @@ -182,7 +182,7 @@ const ButtonCore: React.ForwardRefExoticComponent< | |||
| sharedStyles.endIconWrapperTertiary, | |||
| (focused || hovered) && | |||
| kind !== "primary" && | |||
| sharedStyles.iconWrapperSecondaryHovered, | |||
| buttonStyles.iconWrapperHovered, | |||
There was a problem hiding this comment.
I've moved this to the function where we generate style colors dynamically to be able to support default (progressive) and destructive colors.
| const colorToAction = light | ||
| ? buttonColor === "destructive" | ||
| ? "destructiveLight" | ||
| : "progressiveLight" | ||
| : buttonColor === "destructive" | ||
| ? "destructive" | ||
| : "progressive"; | ||
| const themeColorAction = theme.color[colorToAction]; |
There was a problem hiding this comment.
note: Now that the theme tries to cover all the different aspects of the button states (hover/focus, press, disabled, light), I'm simplifying the logic a bit here to avoid using ternary operators when defining the CSS properties below.
e.g.
Instead of: bg: light ? theme.color.light.background : somethingActionEtc.background
We now use bg: themeColorAction.default.background.
| paddingRight: padding, | ||
| background: themeColorAction.default.background, | ||
| color: themeColorAction.default.foreground, | ||
| paddingInline: padding, |
There was a problem hiding this comment.
note: Switching to Inline here for better RTL support (it shouldn't affect the current styles at all).
| background: themeColorAction.default.background, | ||
| color: themeColorAction.default.foreground, | ||
| borderColor: themeColorAction.default.border, | ||
| borderStyle: "solid", | ||
| borderWidth: theme.border.width.secondary, | ||
| paddingLeft: padding, | ||
| paddingRight: padding, | ||
| paddingInline: padding, |
There was a problem hiding this comment.
note: These are the main changes of the secondary button (resting state). I don't understand why Chromatic is detecting so many changes with the secondary variant.
| ":focus-visible": { | ||
| outlineColor: themeColorAction.disabled.foreground, | ||
| outlineStyle: "solid", | ||
| outlineWidth: theme.border.width.disabled, | ||
| }, |
There was a problem hiding this comment.
note: Adding CSS pseudo-state here.
…This replaces the use of `color` primitive tokens in favor of semantic color tokens
…go theme with new tokens
b79976c to
bf2bbce
Compare
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (2bfd6c9) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2431"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2431 |
There was a problem hiding this comment.
note: I've changed the color tokens structure here to follow a similar approach to the semanticColor actions. Also added a disabled category to every variant b/c Button has particular disabled states (I hope we could really simplify this in the future).
| const focusStyling = { | ||
| outlineColor: light ? theme.color.bg.primary.default : color, | ||
| outlineColor: themeColorAction.default.background, |
There was a problem hiding this comment.
Since this is the focus outline, should this be themeColorAction.default.border instead of background?
| background: light | ||
| ? theme.color.bg.secondary.inverse | ||
| : theme.color.bg.secondary.focus, | ||
| background: themeColorAction.hover.background, | ||
| borderColor: "transparent", |
There was a problem hiding this comment.
Should the border color use a token as well? This is an example where there is a outline and border are different colors!
There was a problem hiding this comment.
I've removed that token as I noticed it was not needed for this case.
| borderColor: themeColorAction.disabled.foreground, | ||
| outlineColor: themeColorAction.disabled.foreground, |
There was a problem hiding this comment.
Should these use the border token instead of foreground?
| cursor: "default", | ||
| ":focus-visible": { | ||
| outlineColor: themeColorAction.disabled.foreground, |
There was a problem hiding this comment.
Should this be border instead of foreground?
| @@ -510,13 +490,18 @@ export const _generateStyles = ( | |||
| focused: focusStyling, | |||
| pressed: activePressedStyling, | |||
| disabled: { | |||
| color: light ? fadedColor : theme.color.text.disabled, | |||
| // NOTE: This is an special case to handle the light color | |||
| color: themeColorAction.disabled.border, | |||
There was a problem hiding this comment.
Is the special case that we use the border token for the text color? For special cases, I wonder if we could avoid one-offs like this by using the primitive token instead? That way, it's relying on the color instead of the semantic meaning. What do you think!
There was a problem hiding this comment.
Nice catch! It was a mistake from my side. I'm now using the correct token.
| progressive: { | ||
| // filled | ||
| ...semanticColor.action.filled.progressive, | ||
| disabled: { |
There was a problem hiding this comment.
Would it be helpful to have the disabled object as part of the semantic color tokens? (I might have asked this when we were pairing but I can't remember 😅)
There was a problem hiding this comment.
This disabled version is very particular to buttons only, so that's the reason why I kept it separately. Once we migrate to the new theme and it becomes default, then we should be able to move this tokens to the global semantic ones.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2431 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
Summary:
Now that we have
semanticColortokens available, this PR is responsible forrefactoring the existing
Buttoncode to use these tokens instead of thecolorprimitive tokens. Also, it modifies the theme structure to followcloser the semantic color tokens structure.
Also added a
light.destructive.disabledvariant to the Dark story as we didn'tinclude a snapshot for it.
Issue: WB-1853
Test plan:
Navigate to
/?path=/docs/packages-button--docsand verify that the buttonhasn't changed.