feat(v3): CheckboxGroup, FieldCheckboxGroup #602
Conversation
ntatoud
added
enhancement
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant FieldCheckboxGroup
participant CheckboxGroup
participant Checkbox
User->>Form: Submit or interact with form
Form->>FieldCheckboxGroup: Render field (via Controller)
FieldCheckboxGroup->>CheckboxGroup: Render group with options
CheckboxGroup->>Checkbox: Render each checkbox
Checkbox->>FieldCheckboxGroup: onChange (user toggles)
FieldCheckboxGroup->>Form: Update form state
Form->>User: Show validation result or submit data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)app/components/ui/checkbox-group.tsx (1)Learnt from: ivan-dalmet 🧬 Code Graph Analysis (1)app/components/ui/checkbox-group.tsx (1)
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ntatoud
force-pushed
the
v3/feat-field-checkbox-group
branch
from
186d08f to
68f9828
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
app/components/ui/checkbox.tsx (1)
44-87: Consider adding aria-invalid support to parent state for consistency.The parent state implementation is comprehensive but lacks
aria-invalidstyling that's present in the default state. This might be needed for form validation consistency.Consider adding aria-invalid styling to the parent state className:
className={cn( 'flex size-5 cursor-pointer items-center justify-center rounded-sm outline-none', 'focus-visible:ring-[3px] focus-visible:ring-ring/50', + 'aria-invalid:focus-visible:ring-destructive/50 aria-invalid:data-unchecked:border-destructive', 'data-checked:bg-primary data-unchecked:border data-unchecked:border-primary/50', 'disabled:cursor-not-allowed disabled:bg-muted-foreground disabled:opacity-20', className )}app/components/form/field-checkbox-group/index.tsx (1)
58-61: Consider removing empty classNameThe container div has an empty className string which serves no purpose.
- className={cn('', containerProps?.className)} + className={cn(containerProps?.className)}app/components/ui/checkbox-group.stories.tsx (1)
101-116: Review the parent-child synchronization logicThe logic for synchronizing parent and child checkbox states could be more robust. Currently, when "bear-3" is selected, it automatically selects all little bears, but the reverse logic might not work as expected in all scenarios.
Consider extracting this logic into a custom hook for better testability and reusability:
const useNestedCheckboxSync = (parentValue: string, childValues: string[]) => { // Implementation for parent-child synchronization };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/form/field-checkbox-group/docs.stories.tsx(1 hunks)app/components/form/field-checkbox-group/index.tsx(1 hunks)app/components/form/field-radio-group/index.tsx(4 hunks)app/components/form/form-field-controller.tsx(3 hunks)app/components/ui/checkbox-group.stories.tsx(1 hunks)app/components/ui/checkbox-group.tsx(1 hunks)app/components/ui/checkbox.tsx(3 hunks)app/components/ui/radio-group.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
app/components/form/field-radio-group/index.tsx (1)
app/components/ui/radio-group.tsx (1)
Radio(26-67)
app/components/ui/checkbox-group.tsx (1)
app/lib/tailwind/utils.ts (1)
cn(4-6)
app/components/ui/checkbox.tsx (1)
app/lib/tailwind/utils.ts (1)
cn(4-6)
app/components/form/field-checkbox-group/index.tsx (5)
app/components/ui/checkbox.tsx (2)
CheckboxProps(8-14)Checkbox(16-117)app/components/form/form-field-controller.tsx (1)
FieldProps(40-47)app/components/ui/checkbox-group.tsx (1)
CheckboxGroup(7-17)app/components/form/form-field.tsx (1)
useFormField(50-56)app/lib/tailwind/utils.ts (1)
cn(4-6)
app/components/ui/checkbox-group.stories.tsx (2)
app/components/ui/checkbox-group.tsx (1)
CheckboxGroup(7-17)app/components/ui/checkbox.tsx (1)
Checkbox(16-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (19)
app/components/form/form-field-controller.tsx (3)
13-16: Import statement follows established patterns.The import is correctly placed and follows the same structure as other field component imports.
61-62: Union type extension is correctly implemented.The FieldCheckboxGroupProps is properly added to the union type, maintaining consistency with the existing field types.
105-106: Switch case implementation follows established pattern.The new case for 'checkbox-group' is correctly implemented and positioned appropriately before the generic comment marker.
app/components/form/field-radio-group/index.tsx (2)
56-56: Good refactoring to centralize invalid state logic.Creating a local
isInvalidconstant eliminates repetition and improves maintainability. The boolean conversion is correct for thearia-invalidattribute.
67-67: Consistent application of invalid state across components.The
isInvalidconstant is properly applied to the RadioGroup, renderOption, and Radio components, ensuring consistent accessibility semantics.Also applies to: 86-86, 95-102
app/components/ui/radio-group.tsx (1)
48-48: Enhanced styling for invalid states improves user experience.The added Tailwind classes provide appropriate visual feedback for invalid radio buttons, complementing the
aria-invalidattribute usage in the form components.app/components/ui/checkbox.tsx (2)
36-40: Good use of UI state management for different checkbox variants.The
getUiStateusage effectively handles the different UI states based on theparentprop, providing a clean separation of concerns.
88-112: Default state maintains existing functionality with enhanced validation styling.The default state implementation preserves the original checkbox behavior while adding appropriate
aria-invalidstyling for form validation.app/components/form/field-checkbox-group/docs.stories.tsx (4)
1-18: LGTM: Clean imports and story setupThe imports are well-organized and the Storybook meta configuration is properly set up for the FieldCheckboxGroup component.
20-26: LGTM: Proper form validation schemaThe Zod schema correctly validates for a non-empty array of strings with a clear error message. The use of
zu.array.nonEmptyis appropriate for checkbox group validation.
28-34: LGTM: Appropriate form configurationThe form options are properly configured with
onBlurmode for validation timing and correct default values for the checkbox group.
42-64: LGTM: Well-structured form storyThe story demonstrates proper integration of the FieldCheckboxGroup within a form context, including proper form field structure with label, helper text, and submission handling.
app/components/form/field-checkbox-group/index.tsx (2)
62-85: LGTM: Excellent accessibility implementationThe CheckboxGroup implementation properly handles all accessibility concerns:
- Correct ARIA attributes linking to form field context
- Proper error state handling in aria-describedby
- Individual checkbox accessibility attributes
- Clean props spreading and value management
49-91: LGTM: Solid Controller integrationThe react-hook-form Controller is properly integrated with all necessary props and correct render prop usage. The field state management and error handling are well implemented.
app/components/ui/checkbox-group.stories.tsx (5)
7-16: LGTM: Clean story setupThe Storybook meta configuration and test data structure are well-organized. The astrobears data includes all necessary properties for demonstrating different checkbox states.
18-28: LGTM: Simple default storyThe default story properly demonstrates basic CheckboxGroup usage with mapped options.
30-68: LGTM: Good coverage of checkbox statesThe DefaultValue, Disabled, and DisabledOption stories provide comprehensive coverage of different checkbox group states and behaviors.
137-145: LGTM: Solid child-to-parent synchronizationThe logic for updating the parent when all children are selected/deselected is well-implemented. The use of
Array.from(new Set([...prev, 'bear-3']))ensures no duplicates are added.
103-168: LGTM: Comprehensive nested group demonstrationThe WithNestedGroups story effectively demonstrates the parent checkbox functionality and complex state management for hierarchical checkbox groups. The visual indentation with
pl-4classes makes the hierarchy clear.
ivan-dalmet
force-pushed
the
v3-main
branch
2 times, most recently
from
cf02ee7 to
cbaf95e
Compare
ntatoud
force-pushed
the
v3/feat-field-checkbox-group
branch
from
649ee50 to
4358f78
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/components/ui/checkbox-group.stories.tsx (2)
12-16: Consider improving mock data consistency.The
grizzlyrinoption is marked asdisabled: truein the mock data but this property isn't used consistently across all stories. Consider either using it in all relevant stories or removing it for clarity.
93-104: Simplify nested bears mock data structure.The
children: undefinedassignments are redundant since undefined is the default value for optional properties in TypeScript.const nestedBears = [ - { value: 'bearstrong', label: 'Bearstrong', children: undefined }, - { value: 'pawdrin', label: 'Buzz Pawdrin', children: undefined }, + { value: 'bearstrong', label: 'Bearstrong' }, + { value: 'pawdrin', label: 'Buzz Pawdrin' }, { value: 'grizzlyrin', label: 'Yuri Grizzlyrin', children: [ { value: 'mini-grizzlyrin-1', label: 'Mini grizzlyrin 1' }, { value: 'mini-grizzlyrin-2', label: 'Mini grizzlyrin 2' }, ], }, ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/ui/checkbox-group.stories.tsx(1 hunks)app/components/ui/checkbox-group.tsx(1 hunks)app/components/ui/checkbox.tsx(1 hunks)app/components/ui/checkbox.utils.tsx(1 hunks)
🧠 Learnings (1)
app/components/ui/checkbox.utils.tsx (1)
Learnt from: ivan-dalmet
PR: #532
File: src/features/auth/PageOAuthCallback.tsx:43-45
Timestamp: 2024-09-30T11:07:14.833Z
Learning: When suggesting changes to useEffect dependencies in React components, ensure that removing dependencies doesn't cause React Hook errors about missing dependencies.
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/ui/checkbox.tsx
- app/components/ui/checkbox-group.tsx
🧰 Additional context used
🧠 Learnings (1)
app/components/ui/checkbox.utils.tsx (1)
Learnt from: ivan-dalmet
PR: #532
File: src/features/auth/PageOAuthCallback.tsx:43-45
Timestamp: 2024-09-30T11:07:14.833Z
Learning: When suggesting changes to useEffect dependencies in React components, ensure that removing dependencies doesn't cause React Hook errors about missing dependencies.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (4)
app/components/ui/checkbox.utils.tsx (2)
8-15: LGTM: Well-structured hook signature with clear TypeScript definitions.The hook interface is well-designed with clear parameter types and optional default values for both main and nested checkbox groups.
53-63: LGTM: Nested change handler logic is sound.The nested group change handler correctly synchronizes the parent state by adding the nested key when all children are selected and removing it otherwise. The use of
Setensures no duplicate values in the main state.app/components/ui/checkbox-group.stories.tsx (2)
70-91: LGTM: Parent checkbox story correctly demonstrates controlled behavior.The story properly demonstrates a parent checkbox with controlled state management, showing how the parent checkbox can control all child checkboxes through the
allValuesprop.
106-149: Excellent demonstration of nested checkbox functionality.This story effectively showcases the complex nested checkbox behavior with proper use of the
useCheckboxGrouphook. The indeterminate state handling and nested group rendering logic are well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/components/ui/checkbox.utils.tsx (1)
36-48: Fix potential state inconsistency in nested checkbox handling.The current logic only clears nested values when all nested checkboxes are checked. This can lead to inconsistent states where the parent is unchecked but some nested checkboxes remain selected.
onValueChange: (value: string[]) => { // Update all nested groups values nestedGroups.forEach((group) => { if (value.includes(group.group)) { group.onValueChange(group.allValues); - } else if (group.value?.length === group.allValues?.length) { + } else { group.onValueChange([]); } }); // Update self value setMainValue(value); },
🧹 Nitpick comments (2)
app/components/ui/checkbox-group.tsx (1)
11-14: Complete the size variants implementation.The size variants are currently empty strings. This TODO should be addressed to provide proper sizing for the checkbox groups.
Would you like me to generate the appropriate Tailwind classes for the size variants or open an issue to track this task?
app/components/ui/checkbox-group.stories.tsx (1)
169-171: Improve debug output formatting.The debug output has an awkward line break. Consider improving the formatting for better readability.
- [{main.value.join(', ')}] <br />[{nested['grizzlyrin']?.value?.join(', ')} - ] + <div> + Main: [{main.value.join(', ')}]<br /> + Grizzlyrin: [{nested['grizzlyrin']?.value?.join(', ')}]<br /> + Pawdrin: [{nested['pawdrin']?.value?.join(', ')}] + </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/form/form-field-controller.tsx(1 hunks)app/components/ui/checkbox-group.stories.tsx(1 hunks)app/components/ui/checkbox-group.tsx(1 hunks)app/components/ui/checkbox.utils.tsx(1 hunks)app/types/utilities.d.ts(1 hunks)
🧠 Learnings (2)
app/components/ui/checkbox-group.tsx (1)
Learnt from: ivan-dalmet
PR: #532
File: src/features/auth/PageOAuthCallback.tsx:43-45
Timestamp: 2024-09-30T11:07:14.833Z
Learning: When suggesting changes to useEffect dependencies in React components, ensure that removing dependencies doesn't cause React Hook errors about missing dependencies.
app/components/ui/checkbox.utils.tsx (1)
Learnt from: ivan-dalmet
PR: #532
File: src/features/auth/PageOAuthCallback.tsx:43-45
Timestamp: 2024-09-30T11:07:14.833Z
Learning: When suggesting changes to useEffect dependencies in React components, ensure that removing dependencies doesn't cause React Hook errors about missing dependencies.
🧬 Code Graph Analysis (2)
app/components/ui/checkbox-group.stories.tsx (3)
app/components/ui/checkbox-group.tsx (1)
CheckboxGroup(48-77)app/components/ui/checkbox.tsx (1)
Checkbox(46-85)app/components/ui/checkbox.utils.tsx (1)
useCheckboxGroup(7-57)
app/components/ui/checkbox.utils.tsx (1)
app/components/ui/checkbox-group.tsx (1)
CheckboxOption(26-30)
🪛 GitHub Check: 🧹 Linter
app/components/ui/checkbox-group.tsx
[warning] 11-11:
Complete the task associated to this "TODO" comment
✅ Files skipped from review due to trivial changes (1)
- app/components/form/form-field-controller.tsx
🧰 Additional context used
🧠 Learnings (2)
app/components/ui/checkbox-group.tsx (1)
Learnt from: ivan-dalmet
PR: #532
File: src/features/auth/PageOAuthCallback.tsx:43-45
Timestamp: 2024-09-30T11:07:14.833Z
Learning: When suggesting changes to useEffect dependencies in React components, ensure that removing dependencies doesn't cause React Hook errors about missing dependencies.
app/components/ui/checkbox.utils.tsx (1)
Learnt from: ivan-dalmet
PR: #532
File: src/features/auth/PageOAuthCallback.tsx:43-45
Timestamp: 2024-09-30T11:07:14.833Z
Learning: When suggesting changes to useEffect dependencies in React components, ensure that removing dependencies doesn't cause React Hook errors about missing dependencies.
🧬 Code Graph Analysis (2)
app/components/ui/checkbox-group.stories.tsx (3)
app/components/ui/checkbox-group.tsx (1)
CheckboxGroup(48-77)app/components/ui/checkbox.tsx (1)
Checkbox(46-85)app/components/ui/checkbox.utils.tsx (1)
useCheckboxGroup(7-57)
app/components/ui/checkbox.utils.tsx (1)
app/components/ui/checkbox-group.tsx (1)
CheckboxOption(26-30)
🪛 GitHub Check: 🧹 Linter
app/components/ui/checkbox-group.tsx
[warning] 11-11:
Complete the task associated to this "TODO" comment
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (3)
app/types/utilities.d.ts (1)
32-52: Well-designed utility types for discriminated unions!The implementation of
OnlyFirst,MergeTypes, andOneOfprovides a robust mechanism for creating mutually exclusive unions from arrays of types. This is particularly useful for theCheckboxGroupcomponent's props wherechildrenandoptionsneed to be mutually exclusive.app/components/ui/checkbox-group.tsx (1)
48-77: Well-structured component with clear separation of concerns!The component elegantly handles both children-based and options-based rendering modes using the discriminated union pattern. The automatic detection of nested groups and proper delegation to sub-components demonstrates good architectural design.
app/components/ui/checkbox-group.stories.tsx (1)
7-173: Comprehensive story coverage demonstrating all component features!The stories effectively showcase the CheckboxGroup component's capabilities including basic usage, disabled states, parent-child relationships, nested groups, and custom state management with the useCheckboxGroup hook. This provides excellent documentation and testing coverage.
app/components/ui/checkbox.utils.tsx
Outdated
| const withoutValuesAndParent = difference(nestedWithParent); | ||
|
|
There was a problem hiding this comment.
Fix incorrect usage of the difference function.
The difference function from remeda requires two arrays but is being called with only one. This creates a partially applied function that's then called incorrectly.
- const nestedWithParent = [...nestedAllValues, parentKey];
- const withoutValuesAndParent = difference(nestedWithParent);
+ const nestedWithParent = [...nestedAllValues, parentKey];
onParentChange((prev) => {
if (areAllChecked) {
return unique([...prev, parentKey, ...newNested]);
}
if (arePartiallyChecked) {
- return unique([...withoutValuesAndParent(prev), ...newNested]);
+ return unique([...difference(prev, nestedWithParent), ...newNested]);
}
- return withoutValuesAndParent(prev);
+ return difference(prev, nestedWithParent);
});Also applies to: 139-143
🤖 Prompt for AI Agents
In app/components/ui/checkbox.utils.tsx around lines 129-130 and 139-143, the
difference function from remeda is incorrectly called with only one array
argument, which results in a partially applied function rather than the intended
array difference. To fix this, ensure difference is called with two array
arguments: the first array and the array of values to exclude from it. Update
the calls to pass both arrays explicitly to correctly compute the difference.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/form/field-checkbox-group/index.tsx (2)
49-49: Remove debug console.log statement.The console.log statement appears to be leftover debug code and should be removed before production.
- console.log(options);
101-101: Remove debug output.The
[{value}]output appears to be debug code and should be removed.- [{value}]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/form/field-checkbox-group/docs.stories.tsx(1 hunks)app/components/form/field-checkbox-group/field-checkbox-group.spec.tsx(1 hunks)app/components/form/field-checkbox-group/index.tsx(1 hunks)app/components/form/form-field-controller.tsx(3 hunks)app/components/ui/checkbox-group.stories.tsx(1 hunks)app/components/ui/checkbox-group.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/form/field-checkbox-group/docs.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/ui/checkbox-group.stories.tsx
- app/components/form/form-field-controller.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/components/form/field-checkbox-group/index.tsx (5)
app/components/ui/checkbox.tsx (2)
CheckboxProps(37-44)Checkbox(46-85)app/components/form/form-field-controller.tsx (1)
FieldProps(40-47)app/components/ui/checkbox-group.tsx (1)
CheckboxGroup(23-40)app/components/form/form-field.tsx (1)
useFormField(50-56)app/lib/tailwind/utils.ts (1)
cn(4-6)
🪛 GitHub Check: 🧹 Linter
app/components/ui/checkbox-group.tsx
[warning] 7-7:
Complete the task associated to this "TODO" comment
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (14)
app/components/ui/checkbox-group.tsx (3)
1-2: Imports look good.The imports are appropriate for a styled UI component using class-variance-authority and the base UI components library.
18-21: Type definitions are well-structured.The props types correctly combine the primitive component props with variant props from CVA.
23-40: Component implementation looks solid.The component correctly applies variant styles, spreads props, and renders children. The implementation follows React best practices.
app/components/form/field-checkbox-group/field-checkbox-group.spec.tsx (8)
1-15: Test setup and data look comprehensive.The imports cover all necessary testing utilities, and the test options provide good coverage including a disabled option for testing various states.
17-43: Accessibility test is well-implemented.The test properly uses vitest-axe to ensure no accessibility violations and includes appropriate mocks for the testing environment.
45-112: Click interaction tests provide excellent coverage.Both direct checkbox clicks and label clicks are tested, which is important for accessibility. The tests properly verify state changes and form submission values.
114-151: Multiple selection test covers essential checkbox group functionality.The test properly verifies that multiple checkboxes can be selected simultaneously and that form submission includes all selected values.
153-187: Keyboard interaction test ensures proper accessibility support.The test verifies that users can navigate to and activate checkboxes using keyboard controls, which is essential for accessibility compliance.
189-220: Default values test ensures proper form integration.The test verifies that the component correctly displays pre-selected values based on form default values, which is crucial for form editing scenarios.
222-254: Disabled group test covers important edge case.The test properly verifies that when the entire checkbox group is disabled, form submission returns undefined, which is the expected behavior for disabled form fields.
256-288: Disabled option test provides comprehensive coverage.The test verifies that individual options can be disabled independently and that disabled options cannot be selected or affect form submission, which is the expected behavior.
app/components/form/field-checkbox-group/index.tsx (3)
1-10: Imports are appropriate and well-organized.All imported modules are necessary for the component's functionality and follow good import organization practices.
12-28: Type definitions are well-structured and comprehensive.The types properly extend base interfaces while adding checkbox group-specific properties. The CheckboxOption type correctly omits conflicting props and adds required label and value fields.
51-107: Component implementation is well-architected.The component properly integrates with react-hook-form using Controller, implements comprehensive accessibility attributes, and handles form state management correctly. The onChange handler appropriately calls both the form's onChange and any additional handlers.
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores