-
Notifications
You must be signed in to change notification settings - Fork 32
feat(GridForm, ConnectedForm): Add nested checkboxes #3168
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 90cb56a
☁️ Nx Cloud last updated this comment at |
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/GridForm/GridFormInputGroup/GridFormNestedCheckboxInput/index.tsx
Show resolved
Hide resolved
packages/styleguide/src/lib/Organisms/ConnectedForm/ConnectedForm/ConnectedForm.stories.tsx
Show resolved
Hide resolved
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 good so far! just have some nits about duped logic and comments. everything is working well minus the useConnectedForm default type + i'd love to see some tests
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
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 some thoughts — the functionality's great!!
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
packages/styleguide/src/lib/Organisms/ConnectedForm/ConnectedForm/ConnectedForm.stories.tsx
Outdated
Show resolved
Hide resolved
packages/styleguide/src/lib/Organisms/ConnectedForm/ConnectedFormInputs/ConnectedFormInputs.mdx
Outdated
Show resolved
Hide resolved
packages/styleguide/src/lib/Organisms/GridForm/GridForm.stories.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes.tsx
Outdated
Show resolved
Hide resolved
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.
Functionality's still great! :)
One thing I did notice when trying the edge-cases is that once I got into a nestedCheckbox with 5 layers (4 worked fine) I got a "Maximum call stack exceeded" in SB.
Not sure if this is SB related or not 🤔
...dForm/ConnectedInputs/ConnectedNestedCheckboxes/__tests__/ConnectedNestedCheckboxes.test.tsx
Outdated
Show resolved
Hide resolved
...dForm/ConnectedInputs/ConnectedNestedCheckboxes/__tests__/ConnectedNestedCheckboxes.test.tsx
Show resolved
Hide resolved
...dForm/ConnectedInputs/ConnectedNestedCheckboxes/__tests__/ConnectedNestedCheckboxes.test.tsx
Outdated
Show resolved
Hide resolved
...s/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes/__tests__/utils.test.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes/index.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes/utils.tsx
Outdated
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes/utils.tsx
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes/utils.tsx
Show resolved
Hide resolved
packages/gamut/src/ConnectedForm/ConnectedInputs/ConnectedNestedCheckboxes/utils.tsx
Show resolved
Hide resolved
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 good to me! would like to mess around with the alpha in author because it has some advanced uses of connectedforms but this works with all the edge-cases i could think of
packages/gamut/src/GridForm/GridFormInputGroup/GridFormNestedCheckboxInput/index.tsx
Outdated
Show resolved
Hide resolved
}, | ||
size: 4, | ||
}, | ||
{ |
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.
may should add a section here on nested checkbox or link the component story. maybe something about the shape of the returned data from the form?
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.
where do you think this fits best? GridForm doesnt currently have any documentation on the fields available... and then ConnectedForm doesn't have any docs on all the customizations lol
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.
started completely revamping the GridForm docs while including this and am going to do it as a follow up
|
||
export interface ConnectedSelectProps | ||
extends Omit<SelectProps, 'defaultValue' | 'name' | 'validation'>, | ||
extends FieldComponent<SelectProps>, |
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.
❤️ the DRYing up here
…nto ajr-nested-checkboxes
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
📬 Published Alpha Packages:
|
🚀 Styleguide deploy preview ready! Preview URL: https://68f10589c2f99c0502e9d992--gamut-preview.netlify.app |
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.
Encountered some VO bug where checking all the children checkboxes visually changes the checkbox but the VO announces the parent as mixed. This happens across all VO tools: VO, NVDA, and JAWS, not sure if this is fixable.
Otherwise, it looks great! well done~
Overview
This PR introduces comprehensive nested checkbox functionality to the
GridForm
andConnectedForm
components, allowing for hierarchical checkbox selections with proper indeterminate states and accessibility support.Features Added
ConnectedNestedCheckboxes
componentConnectedNestedCheckboxes
component and types for use withinConnectedForm
contextsGridFormNestedCheckboxInput
ComponentGridFormNestedCheckboxInput
component and types for use withinGridForm
contextsConnectedNestedCheckboxes
Utility Functions
flattenOptions()
- Converts nested option structure to flat array with level metadatacalculateStates()
- Determines checked/indeterminate states for all optionshandleCheckboxChange()
- Manages cascading checkbox selectionsgetAllDescendants()
- Recursively finds all child optionsrenderCheckbox()
- Renders individual checkboxes with proper indentationAccessibility
aria-checked
states (true, false, mixed)ul
/li
elementslegend
andfieldset
but going to do that as a follow upKey Behaviors
PR Checklist
Testing Instructions
ConnectedForm
GridForm
Accessibility Testing
Use a screen reader (VoiceOver on Mac, NVDA on Windows) to verify:
Test keyboard navigation:
Edge Cases
You can update the playground examples to test the following
spacing: 'tight'
and see the spacing more compactPR Links and Envs