feat(v3): Radio, RadioGroup and FieldRadioGroup #574
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA radio group form field component and its supporting UI primitives are introduced, including Storybook stories and comprehensive tests. The form field system is extended to support radio groups, with improved accessibility via dynamic label and description IDs. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant FieldRadioGroup
participant RadioGroup
participant Radio
participant RHF as ReactHookForm
User->>Form: Submit or interact with radio group
Form->>FieldRadioGroup: Render with control, name, options
FieldRadioGroup->>RHF: Use Controller to manage value
FieldRadioGroup->>RadioGroup: Render radio group UI
RadioGroup->>Radio: Render each radio option
User->>Radio: Select a radio option
Radio->>FieldRadioGroup: onChange event
FieldRadioGroup->>RHF: Update form state
RHF->>Form: Trigger validation and submission
sequenceDiagram
participant Storybook
participant FieldRadioGroup
participant CustomRadio
participant User
Storybook->>FieldRadioGroup: Render with custom renderOption
FieldRadioGroup->>CustomRadio: Use renderOption for each option
User->>CustomRadio: Select option (click)
CustomRadio->>FieldRadioGroup: onChange event
FieldRadioGroup->>Storybook: Form state updates, submit handled
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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 (
|
|
There is an issue on first selection (using label click and not radio click) 
|
ntatoud
force-pushed
the
v3/feat/field-radio
branch
from
cf8d045 to
c76cfdc
Compare
ivan-dalmet
force-pushed
the
v3-main
branch
2 times, most recently
from
939c286 to
d2ff1c8
Compare
ntatoud
force-pushed
the
v3/feat/field-radio
branch
from
bd601e9 to
f34f876
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: 2
♻️ Duplicate comments (1)
app/components/form/field-radio-group/index.tsx (1)
21-25: Consider extracting the RadioOptionProps type.As suggested in past review comments, this type could be extracted to a shared location if it's used elsewhere in the codebase.
-type RadioOptionProps = { +export type RadioOptionProps = { value: string; label: string; disabled?: boolean; -}; +} as const;This makes it reusable and applies readonly constraint as suggested in previous feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/form/field-radio-group/docs.stories.tsx(1 hunks)app/components/form/field-radio-group/field-radio-group.spec.tsx(1 hunks)app/components/form/field-radio-group/index.tsx(1 hunks)app/components/form/form-field-controller.tsx(3 hunks)app/components/ui/radio-group.stories.tsx(1 hunks)app/components/ui/radio-group.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
app/components/form/form-field-controller.tsx (1)
app/components/form/field-radio-group/index.tsx (2)
FieldRadioGroupProps(27-46)FieldRadioGroup(48-142)
app/components/ui/radio-group.stories.tsx (2)
app/components/ui/radio-group.tsx (3)
RadioGroup(74-74)Radio(74-74)RadioItem(74-74)app/components/form/field-radio-group/docs.stories.tsx (4)
Default(41-63)DefaultValue(65-92)Disabled(94-122)Row(124-147)
app/components/ui/radio-group.tsx (1)
app/lib/tailwind/utils.ts (1)
cn(4-6)
app/components/form/field-radio-group/index.tsx (4)
app/components/form/form-field-controller.tsx (1)
FieldProps(32-39)app/components/ui/radio-group.tsx (2)
RadioGroup(74-74)Radio(74-74)app/components/form/form-field.tsx (1)
useFormField(48-54)app/lib/tailwind/utils.ts (1)
cn(4-6)
🪛 GitHub Actions: 🔎 Code Quality
app/components/form/field-radio-group/field-radio-group.spec.tsx
[error] 1-1: Invalid environment variables: 'VITE_BASE_URL' is required but was not provided.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (10)
app/components/form/form-field-controller.tsx (1)
13-13: Clean integration of radio-group support.The addition of
FieldRadioGroupsupport follows the established patterns in the form controller. The import, type definition, and switch case are all correctly implemented and consistent with existing field types.Also applies to: 52-52, 89-90
app/components/ui/radio-group.tsx (2)
9-17: Clean RadioGroup implementation.The RadioGroup component provides a clean wrapper around Radix primitives with sensible defaults and customization options.
23-65: Verify label‐click behavior on first interactionThe
Radiocomponent correctly usesuseId()to generate a uniqueid, applies it toRadioGroupPrimitive.Item, and setshtmlForon the<label>. No other radio implementations were found for comparison.• File to test:
- app/components/ui/radio-group.tsx (lines 23–65)
• Confirm in your dev environment that clicking the label selects the radio on the very first click, per the PR objectives.app/components/ui/radio-group.stories.tsx (1)
17-101: Comprehensive story coverage with good examples.The stories effectively demonstrate all major use cases of the RadioGroup component, including custom styling with RadioItem. The examples are clear and follow Storybook best practices.
app/components/form/field-radio-group/field-radio-group.spec.tsx (1)
30-245: Excellent test coverage for form integration.The test suite provides comprehensive coverage of accessibility, interactions, keyboard navigation, and various states. The use of axe for accessibility testing and thorough keyboard navigation tests demonstrates attention to usability.
app/components/form/field-radio-group/docs.stories.tsx (1)
41-230: Comprehensive form integration documentation.The stories effectively demonstrate all aspects of the FieldRadioGroup component, from basic usage to advanced custom rendering. The validation integration and various configuration options are well-documented, providing clear guidance for developers.
app/components/form/field-radio-group/index.tsx (4)
69-75: LGTM! Clean conditional rendering implementation.The use of
getUiStatefor handling conditional rendering between custom and default radio rendering is well-implemented and provides good separation of concerns.
98-109: LGTM! Proper accessibility implementation.The accessibility attributes are correctly implemented:
aria-invalidproperly reflects validation statearia-describedbycorrectly references description and error IDs- Form field integration follows established patterns
78-88: LGTM! Proper react-hook-form Controller integration.The Controller setup correctly handles all necessary props and properly destructures the render options for form state management.
98-135: Check label-targeting in the Radio componentThe issue isn’t the
onBlurhandler itself but that the<label htmlFor>may not be pointing at an actual<input>element. Radix’sRadioGroupPrimitive.Itemrenders a non-input element (e.g. a div or button withrole="radio"), so on the first click the label doesn’t activate selection.• In app/components/ui/radio-group.tsx (the
Radiofunction), verify which HTML element receives theidand whether it’s a real<input>that a<label>can target.
• In app/components/form/field-radio-group/index.tsx (lines 98–135), confirm that the label’shtmlFormatches a real radio input and selects on first click.
• If it isn’t an<input>, consider one of:
– using<RadioGroupPrimitive.Item asChild>wrapping a native<input type="radio">
– switching to the built-inRadioItemalias or another input-based pattern
– moving your blur logic up to theRadioGrouproot so it doesn’t need per-item listeners
app/components/form/field-radio-group/field-radio-group.spec.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/form/field-radio-group/index.tsx (1)
25-25: Improve type safety by making options readonly.Based on past review feedback, consider making the options array readonly to prevent accidental mutations and improve type safety.
- options: Array<RadioOption>; + options: readonly RadioOption[];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
app/components/form/field-radio-group/docs.stories.tsx(1 hunks)app/components/form/field-radio-group/index.tsx(1 hunks)app/components/ui/radio-group.stories.tsx(1 hunks)app/components/ui/radio-group.tsx(1 hunks)app/tests/setup.ts(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/ui/radio-group.tsx
- app/components/ui/radio-group.stories.tsx
🧰 Additional context used
🧠 Learnings (1)
app/components/form/field-radio-group/index.tsx (2)
Learnt from: ivan-dalmet
PR: BearStudio/start-ui-web#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.
Learnt from: ivan-dalmet
PR: BearStudio/start-ui-web#532
File: src/server/config/oauth/providers/discord.ts:11-11
Timestamp: 2024-10-14T15:29:53.279Z
Learning: In `src/server/config/oauth/providers/discord.ts`, when defining the `zDiscordUser` schema, keep fields like `username` as nullable (e.g., `.nullish()`), as the Discord API might return null values despite the documentation stating otherwise.
🧬 Code Graph Analysis (1)
app/components/form/field-radio-group/index.tsx (4)
app/components/ui/radio-group.tsx (4)
RadioProps(18-23)RadioGroupProps(8-8)RadioGroup(9-16)Radio(24-48)app/components/form/form-field-controller.tsx (1)
FieldProps(32-39)app/components/form/form-field.tsx (1)
useFormField(48-54)app/lib/tailwind/utils.ts (1)
cn(4-6)
🔇 Additional comments (4)
app/tests/setup.ts (1)
16-19: LGTM! Necessary mock for jsdom compatibility.The
scrollTomock is essential for testing environments since jsdom doesn't implement this method. This prevents test failures when components use scrolling functionality.app/components/form/field-radio-group/docs.stories.tsx (1)
1-253: Excellent comprehensive Storybook documentation!The stories effectively demonstrate all key features of the FieldRadioGroup component, including:
- Basic usage with validation
- Default values and disabled states
- Horizontal layout configuration
- Individual disabled options
- Custom rendering with the CardRadio example
The custom CardRadio component demonstrates advanced customization possibilities with proper accessibility handling and visual feedback.
app/components/form/field-radio-group/index.tsx (2)
31-104: Component implementation looks solid overall.The FieldRadioGroup component is well-implemented with proper:
- Form integration via react-hook-form Controller
- Accessibility attributes handling
- Support for both default and custom rendering
- Form field context usage
The component should handle the label clicking issue mentioned in the PR objectives correctly since it properly delegates to the RadioGroup's onValueChange handler.
68-73: No missing descriptionId; context always provides it.The
FormFieldContextValuetype and its provider inapp/components/form/form-field.tsxensuredescriptionIdis always a string (\${id}-description`). You don’t need to guard against an undefinedctx.descriptionId. If the real concern is that you will reference an ID for a description element that the user never rendered, handle that at the rendering site instead (e.g., only emitaria-describedbywhendescription` exists) rather than here.
ntatoud
force-pushed
the
v3/feat/field-radio
branch
from
7527d06 to
083d63a
Compare
ntatoud
force-pushed
the
v3/feat/field-radio
branch
from
ec0f77b to
3a82e32
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/components/form/field-radio-group/index.tsx (1)
25-25: Consider making options array readonly for better type safety.As suggested in past reviews, making the options array readonly prevents accidental mutations and improves type safety.
Apply this diff to enhance type safety:
- options: Array<RadioOption>; + options: readonly RadioOption[];
🧹 Nitpick comments (1)
app/components/form/field-radio-group/index.tsx (1)
16-18: Fix unnecessary type omission in RadioOption.The
RadioOptiontype omits'render'fromRadioProps, but based on the UI component definition,RadioPropsdoesn't have arenderproperty. This omission is unnecessary and potentially confusing.Apply this diff to clean up the type definition:
-type RadioOption = Omit<RadioProps, 'children' | 'render'> & { +type RadioOption = Omit<RadioProps, 'children'> & { label: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/components/form/field-otp/field-otp.spec.tsx(1 hunks)app/components/form/field-radio-group/docs.stories.tsx(1 hunks)app/components/form/field-radio-group/field-radio-group.spec.tsx(1 hunks)app/components/form/field-radio-group/index.tsx(1 hunks)app/components/form/field-text/field-text.spec.tsx(1 hunks)app/components/form/form-field-controller.tsx(3 hunks)app/components/form/form-field-helper.tsx(1 hunks)app/components/form/form-field-label.tsx(1 hunks)app/components/form/form-field.tsx(2 hunks)app/components/ui/radio-group.stories.tsx(1 hunks)app/components/ui/radio-group.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- app/components/form/field-otp/field-otp.spec.tsx
- app/components/form/field-text/field-text.spec.tsx
- app/components/form/form-field-label.tsx
- app/components/form/form-field.tsx
- app/components/form/form-field-controller.tsx
- app/components/ui/radio-group.tsx
- app/components/form/form-field-helper.tsx
- app/components/form/field-radio-group/docs.stories.tsx
- app/components/ui/radio-group.stories.tsx
- app/components/form/field-radio-group/field-radio-group.spec.tsx
🧰 Additional context used
🧠 Learnings (1)
app/components/form/field-radio-group/index.tsx (3)
Learnt from: ivan-dalmet
PR: BearStudio/start-ui-web#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.
Learnt from: ivan-dalmet
PR: BearStudio/start-ui-web#532
File: src/server/config/oauth/providers/discord.ts:11-11
Timestamp: 2024-10-14T15:29:53.279Z
Learning: In `src/server/config/oauth/providers/discord.ts`, when defining the `zDiscordUser` schema, keep fields like `username` as nullable (e.g., `.nullish()`), as the Discord API might return null values despite the documentation stating otherwise.
Learnt from: DecampsRenan
PR: BearStudio/start-ui-web#537
File: src/features/devtools/EnvHintDevPopover.tsx:0-0
Timestamp: 2024-10-11T14:57:53.600Z
Learning: When using `React.cloneElement`, remember that it automatically merges the child's existing `props` with the new props provided, so manually spreading `children.props` is unnecessary.
🧬 Code Graph Analysis (1)
app/components/form/field-radio-group/index.tsx (4)
app/components/ui/radio-group.tsx (4)
RadioProps(18-23)RadioGroupProps(8-8)RadioGroup(9-16)Radio(24-48)app/components/form/form-field-controller.tsx (1)
FieldProps(32-39)app/components/form/form-field.tsx (1)
useFormField(50-56)app/lib/tailwind/utils.ts (1)
cn(4-6)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (3)
app/components/form/field-radio-group/index.tsx (3)
67-81: Excellent accessibility implementation.The ARIA attributes are properly implemented with conditional error handling. The comment explaining why
aria-labelledbyis used instead of the label'sforattribute is particularly helpful for understanding the RadioGroup's behavior.
82-102: Well-structured option rendering with proper key handling.The implementation correctly handles both custom
renderOptionand defaultRadiorendering. The key is properly applied to the React.Fragment wrapper, which fixes the issue mentioned in past reviews. The consistent passing ofonBlurto both custom and default renderers ensures proper form integration.
50-109: Verify label clicking behavior for custom renderOption implementations.Based on the PR objectives, there's a reported issue with label clicking not working on first selection. While the default
Radioimplementation should handle this correctly through its built-in label wrapping, customrenderOptionimplementations might not properly associate labels with radio inputs.Consider adding documentation or examples showing how to properly implement the
renderOptioncallback to ensure label clicking works correctly:// Example of proper renderOption implementation renderOption: ({ label, value, ...props }) => ( <label className="custom-radio-label"> <input type="radio" value={value} {...props} /> {label} </label> )Would you like me to help create comprehensive documentation or examples for the
renderOptioncallback?
ntatoud
force-pushed
the
v3/feat/field-radio
branch
from
7700b59 to
b447a25
Compare
|
I followed the aria guidelines: https://www.w3.org/WAI/ARIA/apg/patterns/radio/
I also squeezed in a small accessibility fix on the descriptionId and labelId for the FormField.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores