fix: allow passing props shared by all checkboxes / radios to use...Group hook#4864
fix: allow passing props shared by all checkboxes / radios to use...Group hook#4864unekinn wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 8450f94 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
b2c0757 to
3a378d2
Compare
3a378d2 to
dea8d73
Compare
caused all three checkboxes to have the same label/description
8475238 to
8450f94
Compare
|
Waiting for reviewing this after we finish web-first docs. |
There was a problem hiding this comment.
Pull request overview
Expands the public surface of useCheckboxGroup and useRadioGroup so callers can pass any CheckboxProps/RadioProps (e.g. variant) at the hook level. Those props are then forwarded to every checkbox/radio via getCheckboxProps/getRadioProps. The www stories are updated to use the hooks instead of hand-rolled controlled state, and a Storybook Disabled story is repointed to Group.args.
Changes:
- Use
MergeRight<CheckboxProps|RadioProps, …>forUseCheckboxGroupProps/UseRadioGroupProps; rebuild agroupPropsrest (minusonChange/error) inside the prop getters and spread it before per-element props. - Migrate www checkbox/radio Group/Outline stories from local
useStatetouseCheckboxGroup/useRadioGroup(also fixing avalue: 'epost'/'email' mismatch inGroupEn); use the hook's newvariantsupport for the Outline stories. - Internal
reactpackageDisabledcheckbox story now extendsGroup.args(matching itsrender: Group) and a changeset is added.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/utilities/hooks/use-radio-group/use-radio-group.ts | Switches props type to MergeRight<RadioProps, …> and forwards shared props through getRadioProps. |
| packages/react/src/utilities/hooks/use-checkbox-group/use-checkbox-group.ts | Same change for the checkbox-group hook. |
| packages/react/src/components/checkbox/checkbox.stories.tsx | Repoints Disabled story args to Group.args to match its renderer. |
| apps/www/app/content/components/radio/radio.stories.tsx | Rewrites Group/Outline stories to use useRadioGroup (incl. variant: 'outline'). |
| apps/www/app/content/components/checkbox/checkbox.stories.tsx | Rewrites Group/Outline stories to use useCheckboxGroup; fixes GroupEn initial value. |
| .changeset/fuzzy-zoos-rhyme.md | Patch-level changeset describing the new hook capability. |
Comments suppressed due to low confidence (2)
packages/react/src/utilities/hooks/use-radio-group/use-radio-group.ts:120
- Inside
getRadioPropsthe new localconst { onChange, error, ...rest } = props;shadows the outer-scopeonChange/erroralready destructured at the top ofuseRadioGroup(lines 83 & 88). It works because they refer to the same underlying values, but the duplication is confusing and the inneronChange/errorare immediately discarded. Consider reusing the variables from the outer scope and computinggroupPropsfromprops(omittingonChangeanderror) once at the top of the hook so the intent is clearer. Same comment applies inuseCheckboxGroupat lines 161–163.
if (props) {
const { onChange, error, ...rest } = props;
groupProps = rest;
}
packages/react/src/utilities/hooks/use-checkbox-group/use-checkbox-group.ts:163
groupPropsstill contains the group-levelvalue(astring[]from the hook's args), and it is spread into the object returned to each checkbox before the explicitvalue: <string>override later in the same object literal. While property-key ordering currently rescues this (the explicitvaluewins), passing astring[]as aCheckboxProps['value']is type-incompatible and the override is easy to break in future edits. Destructure and excludevalue(and arguablychecked) fromgroupPropsalong withonChangeanderror, so a per-checkboxvalueis never at risk of being overwritten by an upstream array. The same concern applies inuseRadioGroupwhere the group-levelvalue: stringis spread in via...groupPropsbefore the explicitvalueoverride.
if (props) {
const { onChange, error, ...rest } = props;
groupProps = rest;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| getRadioProps: (propsOrValue: string | GetRadioProps) => { | ||
| const props = | ||
| let groupProps: | ||
| | Omit<RadioProps, 'aria-label' | 'aria-labelledby'> | ||
| | undefined; | ||
| if (props) { | ||
| const { onChange, error, ...rest } = props; | ||
| groupProps = rest; | ||
| } |
| export type UseCheckboxGroupProps = MergeRight< | ||
| CheckboxProps, | ||
| { | ||
| /** | ||
| * Disables all checkboxes in the group. | ||
| */ | ||
| disabled?: boolean; | ||
| /** | ||
| * Error message for the group. | ||
| * If set, all checkboxes will have `aria-invalid` set to `true`. | ||
| */ | ||
| error?: ReactNode; | ||
| /** | ||
| * Name of the group. | ||
| * If not set, a random id will be generated. | ||
| */ | ||
| name?: string; | ||
| /** | ||
| * Makes all checkboxes in the group read-only. | ||
| * If set, all checkboxes will have `aria-readonly` set to `true`. | ||
| */ | ||
| readOnly?: boolean; | ||
| /** | ||
| * Initial value of the group | ||
| * @default [] | ||
| */ | ||
| value?: string[]; | ||
| /** | ||
| * Makes all checkboxes in the group required. | ||
| * If set, all checkboxes will have `required` set to `true`. | ||
| */ | ||
| required?: boolean; | ||
| /** | ||
| * Callback that is called when the value of the group changes. | ||
| * @param nextValue string[] | ||
| * @param currentValue string[] | ||
| * @returns void | ||
| */ | ||
| onChange?: (nextValue: string[], currentValue: string[]) => void; | ||
| } | ||
| >; |
| export function useRadioGroup(props?: UseRadioGroupProps): useRadioGroupReturn { | ||
| const { | ||
| error, | ||
| readOnly, | ||
| required, | ||
| disabled, | ||
| name, | ||
| onChange, | ||
| value: initialValue = '', | ||
| } = props || {}; |
mimarz
left a comment
There was a problem hiding this comment.
Looks fine to me, just need to fix the conflicts before merge.
Should we do anything about the comments from Copilot? Some of it does make sense 🤔 |
Summary
useCheckboxGroup and useRadioGroup: these hooks now accept all
CheckboxProps/RadioPropsthat don't conflict with the hooks' own props. This can be used to easily set common props likevariantfor all the checkboxes or radios in the group.Checks
pnpm changesetif relevant)