Add theming to Checkbox/Radio #2662
Conversation
🦋 Changeset detectedLatest commit: fed0da3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 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 |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
marcysutton
force-pushed
the
WB-1972-checkradio
branch
from
8b10e3c to
8d98451
Compare
|
Size Change: +659 B (+0.65%) Total Size: 101 kB
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (1cedf6f) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2662"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2662 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-hodwuxpjep.chromatic.com/ Chromatic results:
|
There was a problem hiding this comment.
Nice work so far! Took a first pass at comparing the snapshots with the TB designs and left some comments in the PR and Chromatic. The TB radio button doesn't show up in the Chromatic diffs for some reason, so I didn't leave comments in Chromatic for that component (there are some visual differences though, like the disabled radio button colors!)
| @@ -28,7 +28,8 @@ | |||
| "@khanacademy/wonder-blocks-search-field": "workspace:*", | |||
| "@khanacademy/wonder-blocks-timing": "workspace:*", | |||
| "@khanacademy/wonder-blocks-tokens": "workspace:*", | |||
| "@khanacademy/wonder-blocks-typography": "workspace:*" | |||
| "@khanacademy/wonder-blocks-typography": "workspace:*", | |||
| "@khanacademy/wonder-blocks-form": "workspace:*" | |||
There was a problem hiding this comment.
Ohhh it makes sense that we need wonder-blocks-form in wonder-blocks-dropdown since we import the TextField component in the dropdown package. It is odd that it is only showing an error now since it was importing it before already 🤔
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@khanacademy/wonder-blocks-form": minor | |||
There was a problem hiding this comment.
We can include the tokens package to the changeset too to be sure that it gets updated with the new css variables! (related #2660 (comment))
| height: size / 2, | ||
| width: size / 2, | ||
| borderRadius: border.radius.radius_full, | ||
| top: `calc(${baseStyles.size} * .25 + ${theme.inputWrapper.padding})`, |
There was a problem hiding this comment.
Can we add a comment for why we multiply by .25 😄
| padding: "0px", | ||
| margin: "0px", |
There was a problem hiding this comment.
Could we use sizing.size_0 for spacing? Do we also want to group this into a layout group following the component token structure?
inputWrapper: {
layout: {
padding: sizing.size_0,
margin: sizing.size_0,
}
}
ad96665 to
8a4ea94
Compare
|
Thanks for making these updates! Left some comments in Chromatic! |
…kbox and Radio components
marcysutton
force-pushed
the
WB-1972-checkradio
branch
from
8a4ea94 to
df213b8
Compare
| // Use the global focus style | ||
| ...focusStyles.focus, |
There was a problem hiding this comment.
suggestion: I think you'll need to target the :focus-visible pseudo-class here.
| // Use the global focus style | |
| ...focusStyles.focus, | |
| // Use the global focus style | |
| ...focusStyles.focus[":focus-visible], |
Also this should work a bit better (no need to spread the object):
":focus-visible:not([disabled])": focusStyles.focus[":focus-visible]
There was a problem hiding this comment.
Oddly it is working without focus-visible! Clicking a checkbox doesn't persist a focus style, while the focus style is visible with the keyboard.
| ":focus-visible:not([disabled])": { | ||
| // Use the global focus style | ||
| ...focusStyles.focus, |
There was a problem hiding this comment.
Same suggestion around :focus-visible here.
There was a problem hiding this comment.
Ah thanks, I missed one!
| checkbox: { | ||
| border: { | ||
| radius: { | ||
| default: "3px", |
There was a problem hiding this comment.
question: What happens if you use a valid border_radius token here? would it change too much the layout?
There was a problem hiding this comment.
I wanted to keep the Classic style as close as possible, and it had a 3px radius.
But now that you mention it, we went with the TB style for other characteristics. This was one of the few differences I kept between themes. TB has a 4px radius. Should we check with Design on that one or just change it? I don't think a border-radius will really affect the layout. It might not even be that noticeable.
There was a problem hiding this comment.
I'd say that changing to 4px should be a safe change, but better double-checking with design as you suggest.
| disabled: { | ||
| // the semanticColor isn't working for this icon. | ||
| // our approach changed to use the TB theme in Classic, | ||
| // but the icon foreground color is themed and no longer works. | ||
| // semanticColor for TB: core.border.neutral.subtle (too dark in classic) | ||
| foreground: "#CBCBCD", | ||
| }, |
There was a problem hiding this comment.
suggestion: If this is for the icon, would core.foreground.neutral.subtle work instead? I wonder if this token is worth moving to the theme folder.
There was a problem hiding this comment.
I do think we need to use a different color, as the token in Figma is too dark on the background color.
Bea and I both thought this was too hard to see at the default size (TB screenshot):
This is with core.foreground.neutral.subtle on Classic. I think the checkmark looks better:
I committed that change, thank you for the suggestion!
There was a problem hiding this comment.
And now that I see it, that Label text is suuuper light in Classic too. Disabled components are exempt from contrast requirements, but it still gets into the realm of usability. I think we need some consistency across disabled components and labels, particularly for text colors....but for text sizes too. Maybe we could do an overall polishing effort to make sure compound forms look good with components used together?
There was a problem hiding this comment.
And now that I see it, that Label text is suuuper light in Classic too
Maybe the disabled label text color is something we can theme for these components? I find the current disabled text styling in Classic easier to read!
Maybe we could do an overall polishing effort to make sure compound forms look good with components used together
Agreed! The placement of the error message for CheckboxGroup and RadioGroup is different from how we place error messages under the field in LabeledField. Something we can make more consistent as well!
There was a problem hiding this comment.
I think this is best suited for a follow-up issue on disabled colors as a whole. Here's why: the Classic token uses core.foreground.disabled.default, which has a different value per theme:
- OG:
core.foreground.disabled.defaultisdefault: color.fadedOffBlack32 - TB:
core.foreground.disabled.defaultisdefault: color.gray_50
Maybe I could hard-code a primitive value in the component theme? Or is there a better way?
There was a problem hiding this comment.
I think we want to avoid using primitive values directly! One we way we can approach it is to have a component theme where different semantic tokens are used for OG and TB.
I'm doing something similar for the label in LabeledField:
| {(label || description || errorMessage) && ( | ||
| <Strut size={spacing.small_12} /> | ||
| )} | ||
| <View |
There was a problem hiding this comment.
We might want to go about this a different way... To remove Strut, I wrapped the affected elements in a View and put styling on it instead. But it broke the direct descendant behavior for fieldset > legend. I worked around this by explicitly adding an aria-labelledby attribute onto the fieldset, so it would have an accessible name exposed. But this might be a case where Strut is better suited for the problem.
There was a problem hiding this comment.
what if you move the gap style to the fieldset element directly? that way, there's no need to define an intermediate wrapper so the fieldset > legend connection is not affected (FWIW that fieldset instance uses flex)
There was a problem hiding this comment.
The problem I ran into was fieldset potentially containing multiple elements: StyledLegend, description, and errorMessage. A gap would apply between all of these, rather than at the end. I wanted to group them and apply a margin to the container. I just pushed an update that wraps the children below in a View, and optionally puts marginBlockStart at the top of that instead!
There was a problem hiding this comment.
Okay, that didn't work -- I couldn't keep the flex properties on the children and put a margin on it. I opted to restore Strut and kick the can down the road!
There was a problem hiding this comment.
Thanks for making all the UI adjustments, UI changes look good! I left a suggestion for the disabled label in case you wanted to include it in this PR or in a followup change!
Before approving, I wanted to get your thoughts on the theme structure for the form package since I'll be adding to it too for theming TextArea and TextField!
| ]; | ||
|
|
||
| const wrapperStyle = [theme.inputWrapper, stateStyles.inputWrapper]; |
There was a problem hiding this comment.
What do you think about explicitly creating an aphrodite style for the inputWrapper css properties? In this case, theme.inputWrapper has valid CSS properties so it works out! But if we add other tokens to that structure it may not work. What comes to mind for example is if we add a foreground property to that object, it won't map to a css property!
There was a problem hiding this comment.
Good call. I added that!
| inputWrapper: { | ||
| padding: sizing.size_0, | ||
| margin: sizing.size_0, | ||
| }, |
There was a problem hiding this comment.
Since this theme is for the form package, I am also adding to this structure for shared component theme tokens for TextField and TextArea! Since inputWrapper will only apply to radio/checkboxes, would it make sense to create a group for it? Something like:
export default {
choice: { // <-- new choice group
inputWrapper: {
layout: { // <-- also new, to group the layout properties together
padding: sizing.size_0,
margin: sizing.size_0,
}
}
},
checkbox: { ... },
field: { ... }, // <-- component theme tokens for TextField and TextArea
}Let me know what you think!
There was a problem hiding this comment.
That makes total sense! Thanks Bea!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2662 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This reverts commit a60d5ac.
## Summary: Adds theming support to Checkbox and Radio, including the Thunderblocks theme. Notable differences in TB include: - Increased hover and target click areas (4px) - 4px border-radius instead of 3px - Semantic colors: Component/Input/Selected blue Issue: WB-1972, WB-1974 ## Test plan: 1. Review Test sheets for Checkbox and Radio 2. Test clicking of padded wrapper around both component types 3. Compare to Figma designs - TB: https://www.figma.com/design/EuFu0U7gqc1koXm8ZhlOLp/%E2%9A%A1%EF%B8%8F-Components?node-id=2048-52&t=B2xxLXv7GkDySMja-0 - OG: https://www.figma.com/design/VbVu3h2BpBhH80niq101MHHE/%F0%9F%92%A0-Main-Components?node-id=15197-6798&t=zmIZ176DtJ5rmwyU-0 Author: marcysutton Reviewers: beaesguerra, jandrade, marcysutton Required Reviewers: Approved By: beaesguerra Checks: ✅ 12 checks were successful, ⏭️ 2 checks have been skipped Pull Request URL: #2662
Adds theming support to Checkbox and Radio, including the Thunderblocks theme. Notable differences in TB include: - Increased hover and target click areas (4px) - 4px border-radius instead of 3px - Semantic colors: Component/Input/Selected blue Issue: WB-1972, WB-1974 1. Review Test sheets for Checkbox and Radio 2. Test clicking of padded wrapper around both component types 3. Compare to Figma designs - TB: https://www.figma.com/design/EuFu0U7gqc1koXm8ZhlOLp/%E2%9A%A1%EF%B8%8F-Components?node-id=2048-52&t=B2xxLXv7GkDySMja-0 - OG: https://www.figma.com/design/VbVu3h2BpBhH80niq101MHHE/%F0%9F%92%A0-Main-Components?node-id=15197-6798&t=zmIZ176DtJ5rmwyU-0 Author: marcysutton Reviewers: beaesguerra, jandrade, marcysutton Required Reviewers: Approved By: beaesguerra Checks: ✅ 12 checks were successful, ⏭️ 2 checks have been skipped Pull Request URL: #2662
…2694) ## Summary: This PR includes the following commits: - Add theming to Checkbox/Radio (#2662) Issue: WB-1972 ## Test plan: Author: marcysutton Reviewers: beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ 11 checks were successful, ⏭️ 2 checks have been skipped, ⌛ 1 check is pending Pull Request URL: #2694
Summary:
Adds theming support to Checkbox and Radio, including the Thunderblocks theme.
Notable differences in TB include:
Issue: WB-1972, WB-1974
Test plan: