-
Notifications
You must be signed in to change notification settings - Fork 3
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
Revamp Radio Component with A11Y #142
Conversation
✅ Deploy Preview for vip-design-system-components ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
541eee6
to
c900483
Compare
Thanks for the classes on each item, @djalmaaraujo |
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 amazing, @djalmaaraujo! Left a minor comment.
src/system/Form/Radio.js
Outdated
{ disabled, defaultValue, onChange, name = '', options = [], className, ...props }, | ||
forwardRef | ||
) => { | ||
const renderedOptions = useMemo( |
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.
@djalmaaraujo I wonder if the overhead of useMemo
is beneficial here. I would guess the options array will likely be small (< 5), and I don't see any heavy processing happening inside the loop.
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.
@brunobasto Might not be that much, indeed. The main reason I added it is because, usually, people do onChange
with an inline-function. The parent will make this component re-render often, mainly because it's a form with many interactions.
as="label" | ||
sx={ { display: 'block', mb: 2, color: 'muted' } } | ||
const Label = React.forwardRef( ( { sx, ...rest }, forwardRef ) => ( | ||
<label |
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.
excellent!
9bebd25
to
9cfedda
Compare
Description
We are revamping the
Radio
component. Radio components only make sense to be used with two or more. That's the nature of Radios; you need two or more options.The current implementation lets you choose only one Radio button. In this new implementation, you must pass an array of options (similar to other components). This will provide more clarity and avoid forms with one radio button, and this new implementation is accessible.
BREAKING CHANGE: The new implementation of the
<Radio>
will break current usage. Once you upgrade this component, you need to change the implementation. Follow the Storybook story to understand the new API.Checklist
Steps to Test
npm run dev
.