[WB-1814.3] Refactor Switch to use semantic colors #2441
Conversation
…d Radio components
…to use semanticColor tokens
🦋 Changeset detectedLatest commit: aac58bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Size Change: +77 B (+0.08%) Total Size: 98.1 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-ryinsuinyy.chromatic.com/ Chromatic results:
|
…e `semanticColor` tokens instead of `color`. This will prepare the component for supporting a more global theming approach in the future.
…est now that we use Chromatic snapshots for all variants
There was a problem hiding this comment.
note: As we have been discussing, this is a new file to run visual regression tests on all variants of a component. I auto-approved the first version in Chromatic so reviewers can compare the baseline with the semanticColor changes.
There was a problem hiding this comment.
Chromatic only showed one test and it wasn't a diff for some reason! It looked good to me so I approved it anyways! https://www.chromatic.com/test?appId=5e1bf4b385e3fb0020b7073c&id=679a4e2a99ecb0356d5a8663
| expect(switchWithIcon).toHaveStyle( | ||
| "background-color: rgba(33, 36, 44, 0.5)", | ||
| ); | ||
| expect(switchWithIcon).toHaveStyle( | ||
| "outline: 2px solid rgb(24, 101, 242)", | ||
| ); |
There was a problem hiding this comment.
note: Removed these assertions now that we fully check this via Chromatic snapshots
| ":focus-within": { | ||
| outline: `solid ${theme.size.width.small}px ${theme.color.outline.default}`, | ||
| outlineOffset: theme.size.offset.default, | ||
| }, |
There was a problem hiding this comment.
note: I noticed that this was missing to display the focus outline on disabled switches.
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 (e2b0b50) 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="PR2441"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2441 |
| activeOn: tokens.color.activeBlue, | ||
| off: action.default.border, | ||
| disabledOff: semanticColor.action.disabled.default, | ||
| activeOff: semanticColor.text.secondary, |
There was a problem hiding this comment.
Would it make sense to use the primitive token directly here or a different semantic token? If we were to change the secondary text color value, would we expect things that are not text to change as well?
Same question about using the border color for theme.color.bg.switch.off!
There was a problem hiding this comment.
That's an excellent question! I think using primitive token would make sense here for now.
Side note: I wonder if we should change icon.primary to use the new offBlack72 token. wdyt? should we raise this to Caitlyn?
There was a problem hiding this comment.
I was wondering something similar! At one point, I came across text with an icon in webapp. The text now uses text.secondary (offBlack72), so I thought I'd use icon.secondary for the icon to match it. For icons, secondary is darker than primary! Do we want to swap them so they are consistent with the text tokens (ie. use primary text with primary icon color, use secondary test with secondary icon color)? Also, I wonder what the use cases are for using the secondary icon color (currently offBlack), maybe we don't need both?
I think this is something we can bring up with Caitlyn for the token work!
Summary:
Continuing with the semantic colors migration, this PR covers the
Switchcomponent.
Switchthemes to use semantic colors.All Variantsstory to switch.Implementation plan:
Issue: WB-1814
Test plan: