-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat(react): add indeterminate support to checkbox #1621
feat(react): add indeterminate support to checkbox #1621
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
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.
This is a great start!
As an overview for testing, please take a look at the testing strategies guidelines as there's a few things we're missing.
Additionally, it's not documented (yet) but we're capturing screenshots for all interactive elements. We should include the indeterminate
checkbox as an additional state to capture here: https://github.com/dequelabs/cauldron/blob/develop/packages/react/src/components/Checkbox/screenshots.e2e.tsx
packages/react/src/components/Icon/icons/checkbox-indeterminate.svg
Outdated
Show resolved
Hide resolved
One additional thing that I'm realizing is we're not matching what native checkboxes do. A checkbox can have an |
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.
At a high level, this looks good - thanks for making changes. I will give it a final pass over tomorrow morning.
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 very minor nit, otherwise looks good!
Co-authored-by: Jason <jason@scurker.com>
closes: #479