-
Notifications
You must be signed in to change notification settings - Fork 0
Issue 64 add checkbox group component #73
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
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.
Pull request overview
This PR implements a reusable checkbox component system for the application, addressing issue #64. The implementation provides both standalone checkbox functionality and a grouped checkbox pattern using Radix UI primitives.
- Adds
CheckboxItemandCheckboxGroupcomponents with support for both single and grouped checkbox usage - Integrates Radix UI checkbox primitives with custom styling and state management
- Includes a comprehensive test page demonstrating both single checkbox and checkbox group patterns
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| client/src/components/ui/checkbox.tsx | Core implementation of CheckboxItem and CheckboxGroup components with context-based state management for grouped checkboxes |
| client/src/app/test/checkbox/page.tsx | Demo page showcasing single checkbox and checkbox group usage with form submission example |
| client/package.json | Adds @radix-ui/react-checkbox dependency |
| client/package-lock.json | Lock file update for the new Radix UI checkbox package dependency |
Files not reviewed (1)
- client/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handleSubmit = (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| alert( | ||
| `Submitted data:\nacceptCondition: ${acceptTerms.toString()}\ndates: ${JSON.stringify(dates)}`, |
Copilot
AI
Dec 9, 2025
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 variable name in the alert message is inconsistent: it shows "acceptCondition" but the actual state variable is named "acceptTerms" (line 19). This should be "acceptTerms" to match the variable name.
| `Submitted data:\nacceptCondition: ${acceptTerms.toString()}\ndates: ${JSON.stringify(dates)}`, | |
| `Submitted data:\nacceptTerms: ${acceptTerms.toString()}\ndates: ${JSON.stringify(dates)}`, |
| const isGrouped = !!value && !!context; | ||
| const isChecked = isGrouped ? context.value.includes(value) : checked; | ||
| const handleCheckedChange = isGrouped | ||
| ? (checked: boolean) => context.onChange(value, checked) |
Copilot
AI
Dec 9, 2025
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 handleCheckedChange callback assumes checked is a boolean, but Radix UI's CheckedState type can be true | false | "indeterminate". This may cause runtime errors when an indeterminate state is passed. Consider handling the indeterminate state or casting to boolean: (checked: CheckedState) => context.onChange(value, checked === true)
| ? (checked: boolean) => context.onChange(value, checked) | |
| ? (checked: CheckboxPrimitive.CheckedState) => context.onChange(value, checked === true) |
| <div className="flex items-center gap-1"> | ||
| <CheckboxPrimitive.Root | ||
| ref={ref} | ||
| id={id} | ||
| className={cn( | ||
| "h-6 w-6 rounded-sm", | ||
| "border-2 border-gray-300", | ||
| "data-[state=checked]:border-none data-[state=checked]:bg-bloom-orbit data-[state=checked]:text-primary-foreground", | ||
| "disabled:cursor-not-allowed disabled:opacity-50", | ||
| className, | ||
| )} | ||
| checked={isChecked} | ||
| onCheckedChange={handleCheckedChange} | ||
| {...props} | ||
| > | ||
| <CheckboxPrimitive.Indicator className="flex items-center justify-center"> | ||
| <FaCheck className="h-4 w-4 text-white" /> | ||
| </CheckboxPrimitive.Indicator> | ||
| </CheckboxPrimitive.Root> | ||
| {children} | ||
| </div> |
Copilot
AI
Dec 9, 2025
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 checkbox and label are rendered in separate elements without an explicit wrapper label element. While the htmlFor/id association works, wrapping both in a <label> element would improve click target area and accessibility. Consider wrapping the entire div content in a label element or document that labels should be passed as children and associated via htmlFor.
| value?: string; | ||
| } | ||
|
|
||
| // checkbox item |
Copilot
AI
Dec 9, 2025
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 single-line comment should either be removed or converted to JSDoc format (/** ... */) to document the component, consistent with other UI components in the codebase like radio-group.tsx.
| // checkbox item | |
| /** | |
| * CheckboxItem component for rendering a single checkbox. | |
| * Can be used standalone or within a CheckboxGroup. | |
| */ |
| // Usage: | ||
| // A single checkbox: use [checked, setChecked] = useState<boolean>(initialValue) to get the checkbox status | ||
| // <CheckboxItem | ||
| // checked={checked} | ||
| // onCheckedChange={(checked) => setChecked(checked === true)} // As Radix Output can be true | false | "indeterminate", simple pass setChecked will cause Typescript complaint | ||
| // // optional props can be passed as in standard html checkbox | ||
| // > | ||
| // {children} // add label here | ||
| //</CheckboxItem> | ||
|
|
||
| // A group of checkbox: use [value, setValue] = useState<string[]>(initialValue) to get the selected value | ||
| // <CheckboxGroup value={value} onValueChange={setValue}> | ||
| // <CheckboxItem value={value1}>{children}</CheckboxItem> // value is a necessary field in this case | ||
| // <CheckboxItem value={value2}>{children}</CheckboxItem> | ||
| // </CheckboxGroup> | ||
|
|
Copilot
AI
Dec 9, 2025
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 usage documentation at the top of the file should be formatted as JSDoc comments (/** ... */) instead of regular comments (// ...), consistent with the documentation pattern used in other UI components like radio-group.tsx. This improves IDE support and discoverability.
| // Usage: | |
| // A single checkbox: use [checked, setChecked] = useState<boolean>(initialValue) to get the checkbox status | |
| // <CheckboxItem | |
| // checked={checked} | |
| // onCheckedChange={(checked) => setChecked(checked === true)} // As Radix Output can be true | false | "indeterminate", simple pass setChecked will cause Typescript complaint | |
| // // optional props can be passed as in standard html checkbox | |
| // > | |
| // {children} // add label here | |
| //</CheckboxItem> | |
| // A group of checkbox: use [value, setValue] = useState<string[]>(initialValue) to get the selected value | |
| // <CheckboxGroup value={value} onValueChange={setValue}> | |
| // <CheckboxItem value={value1}>{children}</CheckboxItem> // value is a necessary field in this case | |
| // <CheckboxItem value={value2}>{children}</CheckboxItem> | |
| // </CheckboxGroup> | |
| /** | |
| * Usage: | |
| * | |
| * A single checkbox: use `[checked, setChecked] = useState<boolean>(initialValue)` to get the checkbox status | |
| * ```tsx | |
| * <CheckboxItem | |
| * checked={checked} | |
| * onCheckedChange={(checked) => setChecked(checked === true)} // As Radix Output can be true | false | "indeterminate", simple pass setChecked will cause Typescript complaint | |
| * // optional props can be passed as in standard html checkbox | |
| * > | |
| * {children} // add label here | |
| * </CheckboxItem> | |
| * ``` | |
| * | |
| * A group of checkbox: use `[value, setValue] = useState<string[]>(initialValue)` to get the selected value | |
| * ```tsx | |
| * <CheckboxGroup value={value} onValueChange={setValue}> | |
| * <CheckboxItem value={value1}>{children}</CheckboxItem> // value is a necessary field in this case | |
| * <CheckboxItem value={value2}>{children}</CheckboxItem> | |
| * </CheckboxGroup> | |
| * ``` | |
| */ |
| // <CheckboxItem value={value2}>{children}</CheckboxItem> | ||
| // </CheckboxGroup> | ||
|
|
||
| import * as CheckboxPrimitive from "@radix-ui/react-checkbox"; |
Copilot
AI
Dec 9, 2025
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 "use client" directive is missing at the top of this file. Other similar Radix UI component files in this codebase (e.g., accordion.tsx, select.tsx) include this directive, which is necessary for components that use React hooks like useCallback and useContext in Next.js App Router.
…component to prevent bugs when an indeterminate state is passed as checked.
…xItem component to improve click target area and accessbility. Update test page so label is not needed now.
|
Copilot suggestions all fixed |
ErikaKK
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.
please use shadcn code, if you are making extra checkbox-group component, please place it in components/ directory, thank you!
Change Summary
Add checkbox item and group component. Checkbox can be used alone or within a checkbox group.
Change Form
Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.
Other Information
checkbox_demo.mp4
Related issue