-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
[WB-1814.1] Refactor Checkbox and Radio to use semantic colors #2439
Conversation
…d Radio components
🦋 Changeset detectedLatest commit: 0d9c7f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
Size Change: -54 B (-0.06%) Total Size: 98 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-rpfqdqdrjr.chromatic.com/ Chromatic results:
|
…to use semanticColor tokens
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 (3fef0a8) 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="PR2439" Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2439 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments around semantic token usage!
Things are looking great so far! The semantic tokens make it easier to review styling code changes since we can focus on if the semantics make sense for the use case (rather than the color values. We can check for how it looks in Chromatic!). This will also help us have more consistent styles across components 😄
color={disabled ? offBlack32 : white} | ||
color={ | ||
disabled | ||
? semanticColor.action.disabled.default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I wonder if it would be helpful to have semanticColor.icon.disabled
? We have semanticColor.icon.action
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a great call! I'll add that token in this PR.
@@ -133,15 +139,15 @@ const sharedStyles = StyleSheet.create({ | |||
outline: "none", | |||
boxSizing: "border-box", | |||
borderStyle: "solid", | |||
borderWidth: 1, | |||
borderWidth: border.width.hairline, | |||
borderRadius: 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The border radius can be a token too!
borderRadius: 3, | |
borderRadius: border.radius.small_3, |
backgroundColor: semanticColor.action.disabled.secondary, | ||
borderColor: semanticColor.border.primary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful for there to be background and border tokens for disabled actions? So then this could be:
backgroundColor: semanticColor.action.disabled.secondary, | |
borderColor: semanticColor.border.primary, | |
backgroundColor: semanticColor.action.disabled.background, | |
borderColor: semanticColor.action.disabled.border, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm yeah, the more we discuss this, the more I think we should have at least aborder/bg/fg
definition in action.disabled
. That would make it easier to distinguish the differences between action
and status
tokens. I really hope we can simplify the button tokens soon so we can benefit from this simplified approach where we can only have to define bg
and fg
tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beaesguerra I'm going to keep it this way for now, but I'll ask Design later today to consolidate on a common structure for action
tokens.
|
||
// Focus and hover have the same style. Focus style only shows | ||
// up with keyboard navigation. | ||
":focus-visible": { | ||
boxShadow: `0 0 0 1px ${white}, 0 0 0 3px ${palette.base}`, | ||
outline: `${border.width.thin}px solid ${colorAction.hover.border}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this change to the global focus outline token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I'll add a TODO comment to change this later.
borderWidth: 2, | ||
backgroundColor: error | ||
? semanticColor.status.critical.background | ||
: colorAction.hover.background, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for there to be a background
property for the global focus token as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question.... for now, it will live as semanticColor.border.focus
. We'll see how we define the global focus tokens for handling other things like outlineOffset, width, etc.
@@ -62,7 +60,7 @@ const disabledChecked = { | |||
height: size / 2, | |||
width: size / 2, | |||
borderRadius: "50%", | |||
backgroundColor: offBlack32, | |||
backgroundColor: semanticColor.action.disabled.default, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use semanticColor.action.disabled.background
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see, we have action.disabled.default
and action.disabled.secondary.
for the unselected/selected disabled states. Maybe this would be a case for using action.disabled
and action.disabledInverse
? So it would look like this:
disabled: {
background: ...,
foreground: ...,
border: ...,
},
disabledInverse: {
background: ...,
foreground: ...,
border: ...,
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. We probably don't need an Inverse disabled state as it depends on every component to define that case. As Radio
is very specific, I can add this object that you are proposing at the component level. This would probably be a good case for a component-theme definition for this component.
Here's the figma for reference (which I followed for applying the tokens): https://www.figma.com/design/VbVu3h2BpBhH80niq101MHHE/%F0%9F%92%A0-Main-Components?node-id=15197-6741&t=rQdKVXrPUcAWd6Ac-4
backgroundColor: white, | ||
borderColor: palette.base, | ||
backgroundColor: colorAction.default.background, | ||
borderColor: colorAction.default.foreground, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be colorAction.default.border
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah unfortunately, we can't for now. the default border
is gray
instead of blue
. That's why I had to use foreground
. I'll add a TODO comment to later change this.
borderWidth: size / 4, | ||
|
||
// Focus and hover have the same style. Focus style only shows | ||
// up with keyboard navigation. | ||
":focus-visible": { | ||
boxShadow: `0 0 0 1px ${white}, 0 0 0 3px ${palette.base}`, | ||
outline: `${border.width.thin}px solid ${colorAction.hover.border}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question around if this would use the global focus token!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a TODO comment here.
const fadedBlue = mix(color.fadedBlue16, white); | ||
const activeBlue = color.activeBlue; | ||
const fadedRed = mix(color.fadedRed8, white); | ||
const activeRed = color.activeRed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay glad we can simplify things! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🎉 Thanks for refining the token usage!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2439 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
## Summary: Continuing with the semantic colors migration, this PR covers the `Switch` component. - Refactored the `Switch` themes to use semantic colors. - Added `All Variants` story to switch. - Disabled chromatic snapshots for existing stories (now using All Variants). ### Implementation plan: 1. #2439 2. #2440 3. Switch (current PR) 4. Accordion, Banner, BirthdayPicker 5. IconButton 6. Clickable, Link 7. Modal 8. Popover, Tooltip 9. Pill, Toolbar Issue: WB-1814 ## Test plan: - Navigate to /?path=/story/packages-switch-switch-all-variants--default - Verify that the styles are correct. Author: jandrade Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ gerald, ⏭️ dependabot Pull Request URL: #2441
…c colors (#2446) ## Summary: Next step is to refactor the `Accordion`, `Banner`, and `BirthdayPicker` components to use semantic colors. ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. Accordion, Banner, BirthdayPicker (current PR) 5. IconButton 6. Clickable, Link 7. Modal 8. Popover, Tooltip 9. Pill, Toolbar Issue: WB-1814 ## Test plan: Verify that the Chromatic snapshots are unchanged. Author: jandrade Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2446
## Summary: Next step is to refactor the `IconButton` component to use semantic colors. Besides the migration, this PR also includes the following changes: - Reworked the theme structure to make it closer to the semanticColor structure. - Moved all the color-related CSS properties to a separate function (`getStylesByKind`). - Added `border` to all the states to prepare the work for Polaris as these icon buttons will look similar to the Button ones. ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. IconButton (current PR) 6. Clickable, Link 7. Modal 8. Popover, Tooltip 9. Pill, Toolbar Issue: WB-1814 ## Test plan: Verify that the Chromatic snapshots are unchanged. URL: `/?path=/story/packages-iconbutton-all-variants--default` Author: jandrade Reviewers: jandrade, marcysutton, beaesguerra Required Reviewers: Approved By: marcysutton, beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2449
Summary:
This PR get us closer to prepare form field components to apply a future Polaris theme.
All Variant
stories for Checkbox and Radio components.color
instances to usesemanticColor
inCheckbox
andRadio
.NOTE: Next PR will do the same for the remaining form fields (
TextField
,TextArea
).Issue: WB-1814
Test plan:
Verify that the Checkbox and Radio stories look correct