Design System: Add Switch Component #6799
Conversation
samwhale
added
Type: Enhancement
samwhale
requested review from
barklund,
miina,
BrittanyIRL,
maxyinger and
embarks
|
Size Change: +3.85 kB (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
| @@ -35,7 +35,7 @@ export const focusCSS = (accent, background) => css` | |||
| outline: none; | |||
| box-shadow: ${({ theme }) => | |||
| `0px 0px 0 2px ${background || theme.colors.bg.primary}, 0px 0px 0 4px ${ | |||
| accent || theme.colors.border.focus | |||
| typeof accent === 'string' ? accent : theme.colors.border.focus | |||
There was a problem hiding this comment.
When using focusCSS in a styled component, the first argument will be the props of the component. The function was written with the following type:
type (accent: string, background: string) => string;but when using it the accent is generally an object. Adding a typecheck to make sure accent is a string makes it so that we don't try to style using an object.
| useKeyDownEffect(radioGroupRef, ['space', 'enter'], handleKeyDown, [ | ||
| handleKeyDown, | ||
| value, | ||
| ]); |
There was a problem hiding this comment.
The switch gets the accessibility from being a radio group. Maybe we shouldn't add this here and instead add it where we need it (in the editor) since it adds extra behavior that may not be wanted by all consumers of this component.
There was a problem hiding this comment.
i like having the useKeyDownEffect here if we're expecting switches to have the same behavior across the board. maybe i am reading this comment wrong.
There was a problem hiding this comment.
I think I'm leaning into removing it tbh (and then adding it back in in the editor). This keydown effect adds extra functionality than what the browser gives us.
If this component were ever used in the future but they didn't want space and enter to change the value, they'd have to come into this component and change it first rather than the base component displaying the 'normal' browser interactions
| const handleChange = useCallback( | ||
| (evt) => { | ||
| onChange(evt, evt.target.value === 'true'); | ||
| }, | ||
| [onChange] | ||
| ); |
There was a problem hiding this comment.
I'm making the values of the radio buttons the booleans true and false. However, when they are rendered by react as html elements their values become the strings "true" and "false".
The value on the event is a string, so we need a way to give the boolean value to the onChange event handler.
I don't love this api, but I couldn't see any other way to do this that also exposed the event. Thoughts?
There was a problem hiding this comment.
Yea I think I'm not used to making the values of a radio either true or false. I feel like there are aspects of this that make it feel between a checkbox and a radio because in a sense we're trying to map checked/unchecked to two radio buttons.
I can't think of a better way to do it as a radio tho 🤷
There was a problem hiding this comment.
Would there be any benefit to making it a checkbox? I feel like the arrow keys toggling it might make it weird at that point tho. hmph 🤔
There was a problem hiding this comment.
To me a switch is a non-boolean checkbox (meaning the 2 values could be 1 and 2 or tacos and enchiladas rather than just a true/false state - but only because of values tied to accessibility), which makes it into a 2 input radio group. I think "traditional" radio input values aren't true/false and also expect more than 2 options so the string values make more sense. Given that a switch will always have 2 values and here making them booleans makes the component less tied to wherever it's getting used, it makes sense. At this point, i think the underlying HTML for this should just be what make the most sense semantically and for keyboard interaction. Which I think is what you have. That's my 2 cents!
There was a problem hiding this comment.
I think we're better off keeping it as a radio so that we can always only have one value selected. I updated the values to only call onChange with booleans though. Internally their values will be "ON" and "OFF"
There was a problem hiding this comment.
I respect your decision to not make it "TACOS" & "ENCHILADAS" internally
| */ | ||
| export const Switch = forwardRef(function ( | ||
| { | ||
| className = '', |
There was a problem hiding this comment.
didn't add a mousetrap class here because we may want to use this apart from having that class.
There was a problem hiding this comment.
Everything looks good!
I think the only slight functional issue I was seeing was that it wasn't updating the checked input properly in the rendered output.
Other than that I agree that the onChange is a little weird cus it's like mapping a toggle to a radio, but not sure of a better solution 🤷 . Maybe some ppl better than I with forms could shed some light
| const handleChange = useCallback( | ||
| (evt) => { | ||
| onChange(evt, evt.target.value === 'true'); | ||
| }, | ||
| [onChange] | ||
| ); |
There was a problem hiding this comment.
Yea I think I'm not used to making the values of a radio either true or false. I feel like there are aspects of this that make it feel between a checkbox and a radio because in a sense we're trying to map checked/unchecked to two radio buttons.
I can't think of a better way to do it as a radio tho 🤷
| const handleChange = useCallback( | ||
| (evt) => { | ||
| onChange(evt, evt.target.value === 'true'); | ||
| }, | ||
| [onChange] | ||
| ); |
There was a problem hiding this comment.
Would there be any benefit to making it a checkbox? I feel like the arrow keys toggling it might make it weird at that point tho. hmph 🤔
| > | ||
| {onLabel} | ||
| <HiddenRadioButton | ||
| checked={value} |
There was a problem hiding this comment.
this looks fine to me, but for some reason when I inspect the markup, the checked attribute isn't switching between the selected radio button:
Kapture.2021-03-17.at.09.15.10.mp4
There was a problem hiding this comment.
Did some investigation and chatted with max about it. Here's our findings on this interesting looking bug:
There’s some really interesting talk about this online. It looks like it’s very common with the way people implement checkboxes:
facebook/react#6321 (comment)
facebook/react#3005 (comment)
It's funny - really popular libraries do this too like Material UI
So if we look at the values in the console, we see that checked is getting changed but it’s just not displayed in the tree
This also happens to our other radio buttons (in design system and in the animation panel). We can fix this by adding a key that changes when the value is updated. This would invalidate the mounted component and re-mount a new one.
At the end of the day - in Javascript we use the checked property to determine the state of the checkbox. It is being updated correctly in javascript, but visually in the dom tree it is confusing 😕
| `} | ||
|
|
||
| /* add focus styling on the slider when the hidden input is focused */ | ||
| :focus-within ~ span { |
| const handleChange = useCallback( | ||
| (evt) => { | ||
| onChange(evt, evt.target.value === 'true'); | ||
| }, | ||
| [onChange] | ||
| ); |
There was a problem hiding this comment.
To me a switch is a non-boolean checkbox (meaning the 2 values could be 1 and 2 or tacos and enchiladas rather than just a true/false state - but only because of values tied to accessibility), which makes it into a 2 input radio group. I think "traditional" radio input values aren't true/false and also expect more than 2 options so the string values make more sense. Given that a switch will always have 2 values and here making them booleans makes the component less tied to wherever it's getting used, it makes sense. At this point, i think the underlying HTML for this should just be what make the most sense semantically and for keyboard interaction. Which I think is what you have. That's my 2 cents!
| useKeyDownEffect(radioGroupRef, ['space', 'enter'], handleKeyDown, [ | ||
| handleKeyDown, | ||
| value, | ||
| ]); |
There was a problem hiding this comment.
i like having the useKeyDownEffect here if we're expecting switches to have the same behavior across the board. maybe i am reading this comment wrong.
* remove `mousetrap` class
Wanna see something weird? Check this out: I don't get why it's different in storybook vs the editor when in rtl .... |
|
Added a bug here for tracking: #6809. I won't try to fix the storybook bug here since it's out of scope 😞 |
Context
Add
Switchcomponent to the design system. Matches these designs: https://www.figma.com/file/bMhG3KyrJF8vIAODgmbeqT/Design-System?node-id=3398%3A94450Summary
Adds the
Switchcomponent to the design system.Other changes:
focusCSSwas not rendering the correct focus stylingRelevant Technical Choices
Based on the original Switch here: https://github.com/google/web-stories-wp/blob/main/assets/src/edit-story/components/form/switch.js
To-do
none
User-facing changes
none
Testing Instructions
QA
UAT
Reviews
Does this PR have a security-related impact?
no
Does this PR change what data or activity we track or use?
no
Does this PR have a legal-related impact?
no
Checklist
Type: XYZlabel to the PRPartially addresses #6492