-
Notifications
You must be signed in to change notification settings - Fork 32
feat(ConnectedForm, GridForm): Add aria-controls to nested checkboxes #3211
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
base: main
Are you sure you want to change the base?
Conversation
|
View your CI Pipeline Execution ↗ for commit 26dfebf ☁️ Nx Cloud last updated this comment at |
|
🚀 Styleguide deploy preview ready! Preview URL: https://691b44c9db7b8100c10962e6--gamut-preview.netlify.app |
|
📬 Published Alpha Packages: |
LinKCoding
left a comment
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 a Q for my own edification.
Otherwise, QA'd and it looks great, nice addition of tests too!
| id={checkboxId} | ||
| label={option.label} | ||
| multiline={option.multiline} | ||
| name={checkboxId} |
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.
very much a nit: I don't think this will really affect anyone, but I do wonder if name can be set as the incoming name from the parameters.
That seems to be more like the HTML pattern. With react forms, this info will be sent in a different format, so this is more trying to understand a topic :) Gimme your thoughts please, thanks!
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.
yeah the html pattern seems to like them grouped with the same name since thats how it gets submitted, but thats not the case with react hook form. in rhf it doesnt seem to be used at all so not sure it matters? this example doesn't even include name so maybe it can be omitted? https://www.w3.org/WAI/ARIA/apg/patterns/checkbox/examples/checkbox-mixed/
@sh0ji do you have thoughts?
Overview
Adds aria-controls to checkboxes that control children.
PR Checklist
Testing Instructions
PR Links and Envs