Radio Primitive + Radio Group improvements + A lot of linting #822
Conversation
WalkthroughThis pull request encompasses a comprehensive set of changes across multiple components and files in the CodeRabbit UI library. The modifications primarily focus on code formatting, improving readability, and introducing new components and primitives, particularly in the radio group and radio cards domains. The changes span various aspects of the codebase, including component implementations, stories, tests, and styling. Changes
Sequence DiagramsequenceDiagram
participant User
participant RadioGroup
participant RadioGroupContext
participant RadioGroupItem
User->>RadioGroup: Select radio option
RadioGroup->>RadioGroupContext: Update checked item
RadioGroupContext-->>RadioGroupItem: Notify state change
RadioGroupItem->>RadioGroupItem: Update visual state
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
src/components/ui/Table/tests/Table.test.tsx (1)
11-11: Consider keeping trailing commas for better maintainability.Removing trailing commas in array/object literals can make future modifications more error-prone and create noisier diffs when adding new items. Trailing commas are considered a best practice in modern JavaScript/TypeScript development.
Apply this diff to restore the trailing commas:
- { id: 4, fullName: 'Johnathan White Jr', age: 36, isIntern: 'No' } + { id: 4, fullName: 'Johnathan White Jr', age: 36, isIntern: 'No' }, - { id: 'isIntern', name: 'In Internship' } + { id: 'isIntern', name: 'In Internship' },Also applies to: 18-18
src/components/ui/AlertDialog/AlertDialog.tsx (1)
18-18: Remove or utilize the unused setter to address lint warning
You never callsetIsOpen, which creates a lint warning. If you don’t intend to manage local state here, consider removing the setter to simplify the code.- const [isOpen, setIsOpen] = useState(open); + const isOpen = open;🧰 Tools
🪛 GitHub Check: lint
[warning] 18-18:
'setIsOpen' is assigned a value but never usedsrc/components/ui/RadioGroup/fragments/RadioGroupItem.tsx (2)
5-7: Consider adding an optionalidprop for accessibility clarity.
Currently, the component usesvalueas theidreference for the<label>and<RadioPrimitive>. Using a dedicatedidprop can help avoid potential conflicts ifvaluecontains special characters or if you need a more descriptive or unique identifier.
9-16: Validatename='radio'usage for multiple groups.
Using a hardcoded name='radio' could lead to unintended grouping behavior if multipleRadioGroupItemcomponents share the same page. Consider makingnameconfigurable to support multiple radio groups within the same view.src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (1)
7-10: Add an ARIA role for improved accessibility.
Consider addingrole="radiogroup"to the container element, ensuring screen readers identify the group’s purpose. This is typically recommended for radio group containers.src/core/primitives/Radio/index.tsx (3)
1-1: Remove unuseduseStateimport.
You are importinguseStatebut not using it within this file. Removing unused imports helps keep the codebase tidy.
3-8: Replace theanytype foronClickdata.
Rather than usingany, consider creating a specific interface for the click data (e.g.,{ value: string; checked: boolean; }). This increases type safety for both the component and its consumers.
10-17: UseonChangeinstead ofonClickfor radios.
Relying ononClickcan lead to edge cases if users navigate with the keyboard or use assistive technologies. Adhering to the standardonChangeevent improves compatibility and accessibility of radio inputs.src/core/primitives/Radio/RadioPrimitive.stories.tsx (2)
10-19: Use matchingidandhtmlForattributes for proper label association.
The labels currently usehtmlFor="radio"andhtmlFor="radio2", but theRadioPrimitiveelements do not have correspondingidattributes. Ensuring IDs match thehtmlForhelps screen readers and assistive technologies.<span> - <RadioPrimitive name='radio' value='radio' onClick={() => {}} /> - <label htmlFor='radio'>Radio 1</label> + <RadioPrimitive id='radio1' name='radio' value='radio' onClick={() => {}} /> + <label htmlFor='radio1'>Radio 1</label> </span> <span> - <RadioPrimitive name='radio' value='radio2' onClick={() => {}} /> - <label htmlFor='radio2'>Radio 2</label> + <RadioPrimitive id='radio2' name='radio' value='radio2' onClick={() => {}} /> + <label htmlFor='radio2'>Radio 2</label> </span>
27-30: Refine event handler signature to improve type safety.
Usinganyfor the data argument can be replaced with an explicit type (e.g.,MouseEvent) or a custom event interface if available.-onClick: (data: any) => { +onClick: (data: React.MouseEvent<HTMLButtonElement>) => { console.log('data', data); }src/components/ui/Disclosure/fragments/DisclosureTrigger.tsx (3)
15-18: Consider using a functional update forsetActiveItem.
Using a functional update forsetActiveItemcan safeguard against potential stale closure if other related states are updated asynchronously.- setActiveItem(activeItem === itemValue ? null : itemValue); + setActiveItem((prevActiveItem) => prevActiveItem === itemValue ? null : itemValue);
24-25: Redundant blank line.
You might consider removing this blank line to keep the return block compact, but this is purely stylistic.24 return ( 25-
26-44: Enhance keyboard accessibility.
Consider extending keyboard support for other keys (e.g., 'Home', 'End', 'Enter', or 'Space'), or incorporatingaria-controlsto fully align with WAI-ARIA recommendations for disclosure widgets.src/components/ui/Disclosure/fragments/DisclosureRoot.tsx (1)
18-20: Specify the ref type to avoid confusion
Consider typingdisclosureRefto a more specific type, for exampleHTMLDivElement | null, to avoid potential runtime issues or confusion about the ref’s usage.- const disclosureRef = useRef(null); + const disclosureRef = useRef<HTMLDivElement | null>(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
src/components/tools/Popper/context/usePopper.tsx(1 hunks)src/components/ui/AlertDialog/AlertDialog.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogAction.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogCancel.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx(1 hunks)src/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsx(1 hunks)src/components/ui/Avatar/stories/Avatar.stories.js(2 hunks)src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js(3 hunks)src/components/ui/Badge/Badge.tsx(1 hunks)src/components/ui/BlockQuote/stories/BlockQuote.stories.js(1 hunks)src/components/ui/Button/stories/Button.stories.js(1 hunks)src/components/ui/Code/Code.tsx(1 hunks)src/components/ui/Code/stories/Code.stories.js(1 hunks)src/components/ui/Code/tests/Code.test.tsx(2 hunks)src/components/ui/Disclosure/Disclosure.tsx(1 hunks)src/components/ui/Disclosure/contexts/DisclosureContext.tsx(2 hunks)src/components/ui/Disclosure/contexts/DisclosureItemContext.tsx(2 hunks)src/components/ui/Disclosure/fragments/DisclosureContent.tsx(1 hunks)src/components/ui/Disclosure/fragments/DisclosureItem.tsx(1 hunks)src/components/ui/Disclosure/fragments/DisclosureRoot.tsx(2 hunks)src/components/ui/Disclosure/fragments/DisclosureTrigger.tsx(1 hunks)src/components/ui/Disclosure/stories/Disclosure.stories.js(1 hunks)src/components/ui/Disclosure/tests/Disclosure.test.tsx(2 hunks)src/components/ui/Em/tests/Em.test.js(1 hunks)src/components/ui/Heading/Heading.tsx(1 hunks)src/components/ui/Heading/tests/Heading.test.js(0 hunks)src/components/ui/Kbd/tests/Kbd.test.js(1 hunks)src/components/ui/Progress/Progress.tsx(1 hunks)src/components/ui/Progress/fragments/ProgressIndicator.tsx(1 hunks)src/components/ui/Progress/stories/Progress.stories.js(2 hunks)src/components/ui/Progress/tests/Progress.test.tsx(0 hunks)src/components/ui/RadioGroup/RadioGroup.stories.js(1 hunks)src/components/ui/RadioGroup/RadioGroup.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx(1 hunks)src/components/ui/Switch/stories/Switch.stories.js(1 hunks)src/components/ui/Table/tests/Table.test.tsx(1 hunks)src/components/ui/Toggle/stories/Toggle.stories.js(1 hunks)src/components/ui/Toggle/tests/Toggle.test.js(1 hunks)src/components/ui/ToggleGroup/contexts/toggleContext.tsx(1 hunks)src/components/ui/ToggleGroup/tests/ToggleGroup.test.js(2 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx(1 hunks)src/core/primitives/Radio/RadioPrimitive.stories.tsx(1 hunks)src/core/primitives/Radio/index.tsx(1 hunks)styles/jsTokens/colors.tokens.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/components/ui/Heading/tests/Heading.test.js
- src/components/ui/Progress/tests/Progress.test.tsx
✅ Files skipped from review due to trivial changes (31)
- src/components/ui/Kbd/tests/Kbd.test.js
- src/components/ui/ToggleGroup/contexts/toggleContext.tsx
- src/components/ui/Disclosure/contexts/DisclosureContext.tsx
- src/components/ui/Toggle/tests/Toggle.test.js
- src/components/ui/Progress/fragments/ProgressIndicator.tsx
- src/components/ui/BlockQuote/stories/BlockQuote.stories.js
- src/components/ui/Code/stories/Code.stories.js
- src/components/ui/Badge/Badge.tsx
- src/components/ui/Code/Code.tsx
- src/components/ui/Disclosure/contexts/DisclosureItemContext.tsx
- src/components/tools/Popper/context/usePopper.tsx
- src/components/ui/Progress/stories/Progress.stories.js
- src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js
- src/components/ui/Toggle/stories/Toggle.stories.js
- src/components/ui/Disclosure/Disclosure.tsx
- src/components/ui/AlertDialog/fragments/AlertDialogTrigger.tsx
- src/components/ui/AlertDialog/fragments/AlertDialogAction.tsx
- src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx
- src/components/ui/ToggleGroup/tests/ToggleGroup.test.js
- src/components/ui/Heading/Heading.tsx
- src/components/ui/Em/tests/Em.test.js
- src/components/ui/AlertDialog/fragments/AlertDialogCancel.tsx
- src/components/ui/Avatar/stories/Avatar.stories.js
- src/components/ui/Progress/Progress.tsx
- src/components/ui/Disclosure/fragments/DisclosureContent.tsx
- src/components/ui/Disclosure/tests/Disclosure.test.tsx
- styles/jsTokens/colors.tokens.js
- src/components/ui/Code/tests/Code.test.tsx
- src/components/ui/Switch/stories/Switch.stories.js
- src/components/ui/Button/stories/Button.stories.js
- src/components/ui/Disclosure/stories/Disclosure.stories.js
🧰 Additional context used
🪛 GitHub Check: lint
src/components/ui/AlertDialog/AlertDialog.tsx
[warning] 18-18:
'setIsOpen' is assigned a value but never used
🔇 Additional comments (32)
src/components/ui/Table/tests/Table.test.tsx (1)
Line range hint
1-67: Verify if this file should be included in this PR.There seems to be a discrepancy between the PR title "Radio Primitive + Radio Group improvements" and the changes made to the Table component's test file. Could you please verify if these changes were intended to be part of this PR?
src/components/ui/AlertDialog/fragments/AlertDialogRoot.tsx (1)
26-26: Well-defined overlay click handler
ThehandleOverlayClickfunction is straightforward and effectively invokes theonClickOutsidecallback. This helps keep the component's behavior clean and maintainable.src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (1)
12-16: Confirm the necessity ofcustomRootClass.
WhilecustomClassSwitchercan be useful, verify thatcustomRootClassis necessary for your use case. Maintaining multiple class strategies can sometimes lead to confusion if the usage is not well-documented.src/components/ui/RadioGroup/RadioGroup.stories.js (1)
18-18: Ensure thatitemsis supported inRadioGroupprops.
Your story passes anitemsprop to<RadioGroup>, but confirm that the actual component properly handlesitems. If the component expects child elements or a different prop name, this might lead to unexpected behavior.src/core/primitives/Radio/index.tsx (1)
21-23: Cross-check controlled vs. uncontrolled usage.
You currently rely ondefaultCheckedand anonClickhandler, but your code does not handle acheckedprop explicitly. If you intend a fully controlled radio input, acheckedprop is preferable.src/components/ui/RadioGroup/RadioGroup.tsx (1)
3-4: Confirm import usage and type definition changes.
The newRadioGroupRootandRadioGroupItemimports align with the updated approach. Ensure that any references to the removedtypeandRadioPrimitiveare also cleaned up in related files for consistency.Also applies to: 6-6
src/components/ui/Disclosure/fragments/DisclosureTrigger.tsx (6)
1-4: Consistent imports.
No issues found. The usage of single quotes and consistent import style aligns with the project's convention.
12-13: Context destructuring is clear and concise.
Destructuring these values from theDisclosureContextandDisclosureItemContextkeeps the code clean and maintains clarity.
20-22: Focus handler is succinct.
TheonFocusHandlerusage is straightforward and keeps the code readable.
47-48: Children rendering is straightforward.
No issues with rendering children; the approach is flexible and clear.
50-51: No issues with closing syntax.
Code reads cleanly here.
53-53: Default export looks good.
The component export matches standard React conventions.src/components/ui/Disclosure/fragments/DisclosureRoot.tsx (7)
1-5: Consistent import statement style
Great job maintaining consistent usage of single quotes and organizing import paths for improved readability.
21-22: State initialization for item tracking
InitializingactiveItemandfocusItemstates is a good approach for managing which item is currently active and focused.
35-44: Mirror the same null check as in focusNextItem
Similarly, consider adding a guard condition here as well to handle unexpectednullreferences.
48-59: Well-structured context state
Providing these values and callbacks through Context is a clean, extensible design that simplifies data flow for consumers.
61-70: Accessible usage of role="region" and aria-label
Definingrole="region"and passing a customaria-labelhelps assistive technologies identify this disclosure container. Good job on ensuring accessibility.
75-75: Export aligns with file naming
ExportingDisclosureRootas the default matches the component name, keeping the module cohesive and easily identifiable.
24-33: Avoid relying on non-null assertion to prevent potential runtime errors
disclosureRef.current!bypasses type safety. Even if you're sure it’s never null, adding a small guard check ensures stability and prevents accidental breakages when ref usage evolves.- const batches = getAllBatchElements(disclosureRef.current!); + if (!disclosureRef.current) return; + const batches = getAllBatchElements(disclosureRef.current);✅ Verification successful
Suggestion confirmed - Add guard clause for consistency and safety
The suggestion is valid and aligns with existing patterns in the codebase. The AccordionRoot component already implements this safer pattern with guard clauses in similar focus-handling functions. Additionally, your code currently mixes non-null assertions (!) with optional chaining (?.), making it inconsistent.
src/components/ui/Accordion/fragments/AccordionRoot.tsxuses the safer pattern:if (!accordionRef.current) return;🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find declaration of disclosureRef echo "=== Searching for disclosureRef declaration ===" ast-grep --pattern 'const disclosureRef = useRef' # Find all usages of disclosureRef.current echo -e "\n=== Searching for disclosureRef.current usages ===" rg "disclosureRef\.current" -A 2 -B 2 # Find similar patterns with .current! in the codebase echo -e "\n=== Searching for similar non-null assertions on .current ===" rg "\.current!" -A 2 -B 2 # Look for existing guard clauses echo -e "\n=== Searching for existing guard clauses on refs ===" rg "if.*!.*\.current.*return" -A 2 -B 2Length of output: 3085
src/components/ui/Disclosure/fragments/DisclosureItem.tsx (13)
1-4: Consistent import statements
Switching to single quotes and maintaining alphabetical alignment improves code consistency.
12-13: Well-defined prop types
DefiningDisclosureItemPropsand destructuring them, including defaults, clarifies usage and ensures better type checking.
16-17: Local state for item tracking
CreatingitemValueandisOpenstates helps isolate each disclosure item’s logic, making it clearer if one item is active vs. another.
19-21: SyncingisOpenwithactiveItem
UsinguseEffectto link the localisOpenstate with the parent context’sactiveItemensures proper synchronization and prevents stale state issues.
23-23: useId ensures unique element IDs
LeveraginguseIdavoids collisions in SSR scenarios and helps keep DOM IDs distinct.
27-28: Conditional focus attribute assignment
This pattern for applying data attributes based on focus state is clean and straightforward.
32-37: Focus handling function
Centralizing the focus logic infocusCurrentItemeliminates repetitive code in click and focus handlers.
43-45: Blur handling
The matchingremoveAttributecall helps maintain consistent focus styling across the disclosure items.
48-49: Connect focus on click
CallingfocusCurrentItemin the click handler is a nice addition, ensuring that tabbing or clicking yields the same UX.
52-53: Keep focus logic consistent
Similarly, hooking into the focus event ensures you handle keyboard-based transitions properly.
55-61: Context provider for item-level state
This pattern fosters modularity. Each item can manage its own behavior while still accessing the parent context.
63-76: Semantic markup
Using adivwithrole="region"andaria-expandedis a great way to communicate the disclosure state to assistive technologies.
81-81: Named export remains consistent
Default exportingDisclosureItemaligns well with the file’s purpose.
kotAPI
changed the title
kotAPI
force-pushed
the
kotapi/radio-group-improvements
branch
from
829fbaf to
63cbd96
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
styles/themes/components/radio.scss (1)
13-24: Consider adding focus styles for accessibility.While the base styling is good, ensure keyboard navigation is visually apparent.
input{ width: 20px; height: 20px; position:relative; display:inline-flex; align-items:center; justify-content:center; flex-shrink:0; outline:none; margin:0; border:none; + &:focus-visible { + outline: 2px solid var(--rad-ui-color-accent-400); + outline-offset: 2px; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.eslintrc.cjs(1 hunks)src/components/ui/Avatar/stories/Avatar.stories.js(1 hunks)src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js(2 hunks)src/components/ui/RadioGroup/RadioGroup.stories.js(1 hunks)src/components/ui/RadioGroup/RadioGroup.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx(1 hunks)styles/themes/components/radio.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js
- src/components/ui/Avatar/stories/Avatar.stories.js
- src/components/ui/RadioGroup/RadioGroup.stories.js
- src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx
🔇 Additional comments (7)
.eslintrc.cjs (1)
33-33: Double-check necessity of disabling the rule “import/no-absolute-path”.Disabling this rule permits absolute import paths, which may cause path resolution issues, especially in multi-environment or monorepo setups. If you genuinely require absolute imports, ensure clarity and consistency across the codebase. Otherwise, consider removing this exception to streamline import conventions.
Below is a script to locate any absolute import statements in the project:
src/components/ui/RadioGroup/RadioGroup.tsx (3)
3-10: LGTM! Clean type definition and imports.The type definition is well-structured with proper React types and the imports are organized logically.
12-16: LGTM! Component now supports dynamic children.The component has been improved to support dynamic content through children props, addressing the previous review comment about hardcoded items.
20-21: LGTM! Good use of component composition pattern.The component follows the compound component pattern by exposing Root and Item as static properties, which improves reusability and flexibility.
styles/themes/components/radio.scss (3)
5-5: LGTM! Improved spacing for better visual hierarchy.The increased gap provides better vertical separation between radio items.
8-11: LGTM! Well-structured flex layout.Good use of flexbox for alignment and spacing of radio items.
25-43: Verify color contrast and add vendor prefixes.Two suggestions for the pseudo-element styling:
- Ensure the color contrast between background colors meets WCAG standards
- Add vendor prefixes for transform property
Run this script to check color contrast:
✅ Verification successful
Add vendor prefixes for transform property
The color contrast between accent-400 and accent-900 variants is sufficient across all color scales. However, the transform property needs vendor prefixes for better browser compatibility:
&:after { content: ''; display: inline-block; position: absolute; -webkit-transform: scale(0.4); -moz-transform: scale(0.4); -ms-transform: scale(0.4); transform: scale(0.4); width: 100%; height: 100%; border-radius: 50%; background-color: var(--rad-ui-color-accent-400); }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Extract and analyze color variables used in radio buttons # Search for rad-ui-color variables in the codebase echo "Searching for color variable definitions..." rg --type css "rad-ui-color-accent-[49]00"Length of output: 9372
Script:
#!/bin/bash # Search for actual color values and theme configuration echo "Searching for color values..." rg --type css "rad-ui-color-(gray|mauve|slate)-[49]00.*#" echo -e "\nSearching for default theme configuration..." rg --type css "default-theme|default_theme"Length of output: 279
Script:
#!/bin/bash # Broader search for color values in token files echo "Searching in token files..." fd -e css -e scss "tokens?" -x cat {} | rg -A 1 "color.*400|color.*900" echo -e "\nSearching for hex colors in theme files..." fd -e css -e scss "theme|tokens?" -x cat {} | rg "#[0-9a-fA-F]{3,6}"Length of output: 33129
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
src/core/primitives/Radio/index.tsx (3)
12-14: Consider using consistent attribute naming.The
data-checkedattribute uses a string conversion. Consider using the same pattern for other boolean attributes if they're added in the future.const dataAttributes = { - 'data-checked': checked.toString() + 'data-state': checked ? 'checked' : 'unchecked' };
16-24: Improve type safety in click handler.The click handler needs better type safety:
- Use consistent optional chaining
- Add proper type guard for the event target
const handleOnClick = (event: React.MouseEvent<HTMLInputElement>) => { - const isChecked = event.target?.checked; + if (!(event.target instanceof HTMLInputElement)) return; + const isChecked = event.target.checked; if (typeof onClick === 'function') { onClick({ value, checked: isChecked }); } };
26-34: Maintain consistency with click handler implementation.The change handler should follow the same type safety patterns as the click handler.
const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => { + if (!(event.target instanceof HTMLInputElement)) return; const isChecked = event.target.checked; if (typeof onChange === 'function') { onChange({ value, checked: isChecked }); } };src/components/ui/RadioGroup/RadioGroup.stories.js (2)
50-53: Remove debug console.logsDebug console.logs should not be committed to production code.
const handleFormSubmit = (e) => { e.preventDefault(); - console.log('submit', language); };
57-65: Add form validationThe form lacks validation to ensure a language is selected before submission. Consider adding the required attribute or validation logic.
<form onSubmit={handleFormSubmit}> - <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} > + <RadioGroup.Root + checked={language} + items={options} + onChange={handleChange} + required + > {options.map((option) => ( <RadioGroup.Item key={option.id} value={option.value}> {option.label} </RadioGroup.Item> ))} </RadioGroup.Root> - <button type="submit">Submit</button> + <button type="submit" disabled={!language}>Submit</button> </form>src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx (1)
6-8: Consider adding description prop for accessibilityTo support more descriptive labels or help text, consider adding an optional description prop.
type RadioGroupItemProps = PropsWithChildren<{ value: string; + description?: string; }>;src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (1)
20-26: Memoize onChange handler for performanceConsider memoizing the onChange handler to prevent unnecessary re-renders.
+ const handleOnChange = useCallback((item: string) => { - const handleOnChange = (item: string) => { setCheckedItem(item); if (typeof onChange === 'function') { onChange(item); } - }; + }, [onChange]);styles/themes/components/radio.scss (2)
8-11: Use relative units for better responsivenessConsider using relative units (rem) instead of fixed pixel values for better scaling.
.rad-ui-radio-group-item{ display: flex; align-items: center; - gap: 12px; + gap: 0.75rem;
25-32: Add high contrast supportConsider adding support for high contrast mode for better accessibility.
&:before{ content: ''; display: inline-block; width: 20px; height: 20px; border-radius: 50%; background-color: var(--rad-ui-color-accent-900); + @media (forced-colors: active) { + background-color: ButtonText; + border: 1px solid ButtonText; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/ui/RadioGroup/RadioGroup.stories.js(2 hunks)src/components/ui/RadioGroup/context/RadioGroupContext.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx(1 hunks)src/core/primitives/Radio/index.tsx(1 hunks)styles/themes/components/radio.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/RadioGroup/context/RadioGroupContext.tsx
🔇 Additional comments (3)
src/core/primitives/Radio/index.tsx (1)
11-11: Review props spreading order.The current implementation spreads custom props before data attributes, which might prevent overriding attributes like
data-checked. Consider reordering the props spreading to ensure proper precedence.-const RadioPrimitive = ({ name = '', value = '', checked = false, onClick, onChange, ...props }:RadioPrimitiveProps) => { +const RadioPrimitive = ({ name = '', value = '', checked = false, onClick, onChange, ...props }:RadioPrimitiveProps) => {src/components/ui/RadioGroup/RadioGroup.stories.js (2)
43-48: Clarify event handling implementationThe
handleChangefunction seems to have inconsistent parameter handling. The parameter is nameddatabut the usage suggests it might be an event object.Could you clarify the expected shape of the onChange callback data from RadioGroup.Root?
Line range hint
1-1: Verify compound component implementationLet's verify that the RadioGroup component properly implements the compound component pattern with proper context sharing.
✅ Verification successful
Compound component implementation verified successfully
The RadioGroup component correctly implements the compound component pattern with proper context sharing between Root and Item components. The implementation includes:
- Namespaced subcomponents
- Context-based state sharing
- Proper composition in stories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check RadioGroup implementation for proper compound component pattern # Look for context implementation echo "Checking for RadioGroup context..." rg -l "createContext.*RadioGroup" src/components/ui/RadioGroup/ # Verify Root component implements context provider echo "Verifying Root component context provider..." ast-grep --pattern 'const RadioGroupRoot = ({ children }) => { $$$ return <$_Provider value={$_}> $$$ </$_Provider> }' # Check for proper component composition echo "Checking component composition..." rg -A 5 "RadioGroup\s*=\s*{" src/components/ui/RadioGroup/RadioGroup.tsxLength of output: 626
Script:
#!/bin/bash # Check the RadioGroup implementation file content echo "=== Checking RadioGroup implementation ===" cat src/components/ui/RadioGroup/RadioGroup.tsx || true echo -e "\n=== Checking for Context usage ===" rg "useContext.*RadioGroup" src/components/ui/RadioGroup/ || true echo -e "\n=== Looking for compound components ===" rg "export const Radio(Group|Item|Option)" src/components/ui/RadioGroup/ || true echo -e "\n=== Checking story file structure ===" cat src/components/ui/RadioGroup/RadioGroup.stories.js || trueLength of output: 3769
Script:
#!/bin/bash echo "=== Checking RadioGroupRoot implementation ===" cat src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx || true echo -e "\n=== Checking RadioGroupContext ===" cat src/components/ui/RadioGroup/fragments/RadioGroupContext.tsx || trueLength of output: 1572
| type RadioPrimitiveProps = { | ||
| onClick: (data: any) => void; | ||
| onChange: (data: any) => void; | ||
| checked: boolean; | ||
| name: string; | ||
| value: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and prop definitions.
The type definition needs the following improvements:
- Replace
anywith a proper type definition for event handler data - Mark props with defaults as optional
- Extend from HTML input props
+type RadioEventData = {
+ value: string;
+ checked: boolean;
+};
+
type RadioPrimitiveProps = {
- onClick: (data: any) => void;
- onChange: (data: any) => void;
- checked: boolean;
- name: string;
- value: string;
+ onClick?: (data: RadioEventData) => void;
+ onChange?: (data: RadioEventData) => void;
+ checked?: boolean;
+ name?: string;
+ value?: string;
+} & Omit<React.InputHTMLAttributes<HTMLInputElement>, 'onClick' | 'onChange' | 'checked' | 'name' | 'value'>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type RadioPrimitiveProps = { | |
| onClick: (data: any) => void; | |
| onChange: (data: any) => void; | |
| checked: boolean; | |
| name: string; | |
| value: string; | |
| }; | |
| type RadioEventData = { | |
| value: string; | |
| checked: boolean; | |
| }; | |
| type RadioPrimitiveProps = { | |
| onClick?: (data: RadioEventData) => void; | |
| onChange?: (data: RadioEventData) => void; | |
| checked?: boolean; | |
| name?: string; | |
| value?: string; | |
| } & Omit<React.InputHTMLAttributes<HTMLInputElement>, 'onClick' | 'onChange' | 'checked' | 'name' | 'value'>; |
src/core/primitives/Radio/index.tsx
Outdated
| <> | ||
| <input id={value} type='radio' value={value} name={name} onClick={handleOnClick} onChange={handleOnChange} {...props} {...dataAttributes} /> | ||
| </> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and props handling.
The render implementation needs improvements:
- Add proper accessibility attributes
- Fix props spreading order to ensure proper overrides
<>
- <input id={value} type='radio' value={value} name={name} onClick={handleOnClick} onChange={handleOnChange} {...props} {...dataAttributes} />
+ <input
+ type='radio'
+ role='radio'
+ aria-checked={checked}
+ {...props}
+ id={value}
+ value={value}
+ name={name}
+ onClick={handleOnClick}
+ onChange={handleOnChange}
+ {...dataAttributes}
+ />
</>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <> | |
| <input id={value} type='radio' value={value} name={name} onClick={handleOnClick} onChange={handleOnChange} {...props} {...dataAttributes} /> | |
| </> | |
| <> | |
| <input | |
| type='radio' | |
| role='radio' | |
| aria-checked={checked} | |
| {...props} | |
| id={value} | |
| value={value} | |
| name={name} | |
| onClick={handleOnClick} | |
| onChange={handleOnChange} | |
| {...dataAttributes} | |
| /> | |
| </> |
| export const InForm = InFormTemplate.bind({}); | ||
| InForm.args = { | ||
| defaultValue: 'css' | ||
| }; |
There was a problem hiding this comment.
Remove unused story args
The defaultValue arg doesn't correspond to any prop used in the InFormTemplate component. If you want to set an initial value, update the useState initialization in the template or use the correct prop name.
export const InForm = InFormTemplate.bind({});
-InForm.args = {
- defaultValue: 'css'
-};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const InForm = InFormTemplate.bind({}); | |
| InForm.args = { | |
| defaultValue: 'css' | |
| }; | |
| export const InForm = InFormTemplate.bind({}); |
| <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} > | ||
| {options.map((option) => ( | ||
| <RadioGroup.Item key={option.id} value={option.value}> | ||
| {option.label} | ||
| </RadioGroup.Item> | ||
| ))} | ||
| </RadioGroup.Root> |
There was a problem hiding this comment.
Convert to controlled component pattern
The RadioGroup.Root is mixing controlled and uncontrolled patterns. Since you're managing state with useState and handling changes with onChange, you should use the checked prop instead of defaultChecked.
- <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} >
+ <RadioGroup.Root checked={language} items={options} onChange={handleChange} >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} > | |
| {options.map((option) => ( | |
| <RadioGroup.Item key={option.id} value={option.value}> | |
| {option.label} | |
| </RadioGroup.Item> | |
| ))} | |
| </RadioGroup.Root> | |
| <RadioGroup.Root checked={language} items={options} onChange={handleChange} > | |
| {options.map((option) => ( | |
| <RadioGroup.Item key={option.id} value={option.value}> | |
| {option.label} | |
| </RadioGroup.Item> | |
| ))} | |
| </RadioGroup.Root> |
| <div className="rad-ui-radio-group-item"> | ||
| <RadioPrimitive value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} /> | ||
| <label htmlFor={value}>{children}</label> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility implementation
The radio group item needs better accessibility support:
- The label's
htmlForwon't work as expected since we're not exposing the actual input's ID - Missing ARIA attributes for better screen reader support
Apply these changes:
<div className="rad-ui-radio-group-item">
- <RadioPrimitive value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} />
- <label htmlFor={value}>{children}</label>
+ <RadioPrimitive
+ id={`radio-${value}`}
+ value={value}
+ name='radio'
+ checked={checkedItem === value}
+ onChange={handleOnChange}
+ aria-labelledby={`radio-label-${value}`}
+ />
+ <label
+ id={`radio-label-${value}`}
+ htmlFor={`radio-${value}`}
+ >
+ {children}
+ </label>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="rad-ui-radio-group-item"> | |
| <RadioPrimitive value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} /> | |
| <label htmlFor={value}>{children}</label> | |
| </div> | |
| <div className="rad-ui-radio-group-item"> | |
| <RadioPrimitive | |
| id={`radio-${value}`} | |
| value={value} | |
| name='radio' | |
| checked={checkedItem === value} | |
| onChange={handleOnChange} | |
| aria-labelledby={`radio-label-${value}`} | |
| /> | |
| <label | |
| id={`radio-label-${value}`} | |
| htmlFor={`radio-${value}`} | |
| > | |
| {children} | |
| </label> | |
| </div> |
| type RadioGroupRootProps = PropsWithChildren<{ | ||
| className?: string; | ||
| customRootClass?: string; | ||
| defaultChecked?: string; | ||
| onChange?: (item: string) => void; | ||
| }>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-label prop for accessibility
The radio group should have a descriptive label for screen readers.
type RadioGroupRootProps = PropsWithChildren<{
className?: string;
customRootClass?: string;
defaultChecked?: string;
onChange?: (item: string) => void;
+ 'aria-label': string;
}>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type RadioGroupRootProps = PropsWithChildren<{ | |
| className?: string; | |
| customRootClass?: string; | |
| defaultChecked?: string; | |
| onChange?: (item: string) => void; | |
| }>; | |
| type RadioGroupRootProps = PropsWithChildren<{ | |
| className?: string; | |
| customRootClass?: string; | |
| defaultChecked?: string; | |
| onChange?: (item: string) => void; | |
| 'aria-label': string; | |
| }>; |
| onChange: handleOnChange | ||
| }; | ||
|
|
||
| return <RadioGroupContext.Provider value={sendItems}><div className={clsx(rootClass, className)}>{children}</div></RadioGroupContext.Provider>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add required ARIA attributes for radio group
The root element needs proper ARIA attributes for accessibility.
- return <RadioGroupContext.Provider value={sendItems}><div className={clsx(rootClass, className)}>{children}</div></RadioGroupContext.Provider>;
+ return (
+ <RadioGroupContext.Provider value={sendItems}>
+ <div
+ role="radiogroup"
+ aria-label="radio group"
+ className={clsx(rootClass, className)}
+ >
+ {children}
+ </div>
+ </RadioGroupContext.Provider>
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return <RadioGroupContext.Provider value={sendItems}><div className={clsx(rootClass, className)}>{children}</div></RadioGroupContext.Provider>; | |
| return ( | |
| <RadioGroupContext.Provider value={sendItems}> | |
| <div | |
| role="radiogroup" | |
| aria-label="radio group" | |
| className={clsx(rootClass, className)} | |
| > | |
| {children} | |
| </div> | |
| </RadioGroupContext.Provider> | |
| ); |
| input{ | ||
| width: 20px; | ||
| height: 20px; | ||
| position:relative; | ||
| display:inline-flex; | ||
| align-items:center; | ||
| justify-content:center; | ||
| flex-shrink:0; | ||
| outline:none; | ||
| margin:0; | ||
| border:none; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add focus and hover states for better accessibility
The radio inputs are missing visible focus and hover states.
input{
width: 20px;
height: 20px;
position:relative;
display:inline-flex;
align-items:center;
justify-content:center;
flex-shrink:0;
outline:none;
margin:0;
border:none;
+ cursor: pointer;
+
+ &:focus-visible::before {
+ outline: 2px solid var(--rad-ui-color-accent-400);
+ outline-offset: 2px;
+ }
+
+ &:hover::before {
+ background-color: var(--rad-ui-color-accent-800);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| input{ | |
| width: 20px; | |
| height: 20px; | |
| position:relative; | |
| display:inline-flex; | |
| align-items:center; | |
| justify-content:center; | |
| flex-shrink:0; | |
| outline:none; | |
| margin:0; | |
| border:none; | |
| input{ | |
| width: 20px; | |
| height: 20px; | |
| position:relative; | |
| display:inline-flex; | |
| align-items:center; | |
| justify-content:center; | |
| flex-shrink:0; | |
| outline:none; | |
| margin:0; | |
| border:none; | |
| cursor: pointer; | |
| &:focus-visible::before { | |
| outline: 2px solid var(--rad-ui-color-accent-400); | |
| outline-offset: 2px; | |
| } | |
| &:hover::before { | |
| background-color: var(--rad-ui-color-accent-800); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (3)
src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx (1)
6-9: 🛠️ Refactor suggestionImprove accessibility and type safety.
The implementation needs accessibility improvements as mentioned in the previous review, and the props type could be more specific.
+interface RadioGroupItemProps extends RadioGroupPrimitive.ItemProps { + children: React.ReactNode; + className?: string; +} -const RadioGroupItem = ({ children, className = '', ...props }: { children: React.ReactNode } & RadioGroupPrimitive.ItemProps) => { +const RadioGroupItem = ({ children, className = '', value, ...props }: RadioGroupItemProps) => { const { defaultChecked, onChange, rootClass } = useContext(RadioGroupContext); - return <RadioGroupPrimitive.Item className={clsx(`${rootClass}-item`, className)} {...props}>{children}</RadioGroupPrimitive.Item>; + return ( + <RadioGroupPrimitive.Item + className={clsx(`${rootClass}-item`, className)} + id={`radio-${value}`} + value={value} + aria-labelledby={`radio-label-${value}`} + {...props} + > + <label id={`radio-label-${value}`} htmlFor={`radio-${value}`}> + {children} + </label> + </RadioGroupPrimitive.Item> + ); };src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (2)
11-18: 🛠️ Refactor suggestionAdd accessibility props and documentation.
The type definition needs accessibility-related props and JSDoc documentation.
+/** + * Props for the RadioGroupRoot component + * @param children - The radio group items + * @param className - Additional CSS classes + * @param defaultChecked - The default selected value + * @param onChange - Callback when selection changes + * @param customRootClass - Custom root class for styling + * @param aria-label - Accessible label for the radio group + */ type RadioGroupRootProps = { children: React.ReactNode; className?: string; defaultChecked?: string | null; onChange?: (e: React.ChangeEvent<HTMLInputElement>) => void; customRootClass?: string; + 'aria-label': string; + 'aria-describedby'?: string; };
19-25: 🛠️ Refactor suggestionAdd accessibility attributes and optimize performance.
The component needs accessibility improvements and performance optimizations.
-const RadioGroupRoot = ({ children, className = '', defaultChecked = null, onChange = null, customRootClass = '' }: RadioGroupRootProps) => { +const RadioGroupRoot = React.memo(({ + children, + className = '', + defaultChecked = null, + onChange = null, + customRootClass = '', + 'aria-label': ariaLabel, + 'aria-describedby': ariaDescribedBy +}: RadioGroupRootProps) => { const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); + const handleChange = React.useCallback((e: React.ChangeEvent<HTMLInputElement>) => { + onChange?.(e); + }, [onChange]); return <RadioGroupContext.Provider value={{ defaultChecked, rootClass, onChange }}> - <RadioGroupPrimitive.Root className={clsx(rootClass, className)} customRootClass={customRootClass}>{children}</RadioGroupPrimitive.Root> + <RadioGroupPrimitive.Root + className={clsx(rootClass, className)} + customRootClass={customRootClass} + role="radiogroup" + aria-label={ariaLabel} + aria-describedby={ariaDescribedBy} + onChange={handleChange} + > + {children} + </RadioGroupPrimitive.Root> </RadioGroupContext.Provider>; -}; +}); + +RadioGroupRoot.displayName = 'RadioGroupRoot';
🧹 Nitpick comments (14)
src/components/ui/RadioCards/fragments/RadioCardsItem.tsx (1)
8-8: Remove unused context values.The component destructures
defaultCheckedandonChangefrom context but never uses them. Remove unused values to improve code clarity.- const { defaultChecked, onChange, rootClass } = useContext(RadioCardsContext); + const { rootClass } = useContext(RadioCardsContext);src/components/ui/RadioCards/fragments/RadioCardsRoot.tsx (1)
23-23: Remove duplicate customRootClass prop.The
customRootClassis already processed bycustomClassSwitcherand included in therootClass. Passing it again toRadioGroupPrimitive.Rootis redundant.- <RadioGroupPrimitive.Root className={clsx(rootClass, className)} customRootClass={customRootClass}>{children}</RadioGroupPrimitive.Root> + <RadioGroupPrimitive.Root className={clsx(rootClass, className)}>{children}</RadioGroupPrimitive.Root>src/components/ui/RadioCards/RadioCards.stories.tsx (1)
5-5: Add TypeScript types for args parameter.The
argsparameter lacks type information. Consider adding proper typing for better type safety.- const RadioCardsTemplate = (args) => { + const RadioCardsTemplate = (args: Record<string, unknown>) => {styles/themes/components/radiocards.scss (2)
26-29: Enhance keyboard navigation with :focus-visible.Consider using
:focus-visibleinstead of:focus-withinfor better keyboard navigation support. Also, add hover state styles for better user interaction feedback.- &:focus-within { + &:hover { + background-color: var(--rad-ui-color-accent-50); + } + + &:focus-visible { border: 1px solid var(--rad-ui-color-accent-800); background-color: var(--rad-ui-color-accent-100); }
16-16: Consider using CSS custom property for border-radius.For consistency and easier theme customization, consider using a CSS custom property for the border-radius value.
- border-radius: 8px; + border-radius: var(--rad-ui-border-radius-md, 8px);styles/themes/default.scss (1)
Line range hint
1-85: Consider cleaning up the file structure.The file contains several areas that could be improved:
- Remove or implement the commented-out focus styles (lines 34-40).
- Remove excessive empty lines between sections.
- Clean up empty or incomplete classes (
.rad-ui-tooltip,.rad-ui-tooltip-reference-element,.rad-ui-popper).src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (2)
6-9: Consider strengthening the type definition.The
RadioGroupPropstype could be more specific:
- Consider removing
PropsWithChildrensince it's redundant with thechildrenprop- Add specific radio-related props like
value,defaultValue, etc.export type RadioGroupProps = { children?: React.ReactNode; + value?: string; + defaultValue?: string; + onChange?: (value: string) => void; } & DetailedHTMLProps<InputHTMLAttributes<HTMLInputElement>, HTMLInputElement> - & PropsWithChildren
11-14: Add JSDoc documentation and consider explicit typing.The component initialization could be more explicit and documented:
-const RadioGroupPrimitive = {} as const; +/** + * A primitive radio group component that follows the compound component pattern. + * Use RadioGroupPrimitive.Root as the container and RadioGroupPrimitive.Item for individual radio options. + */ +const RadioGroupPrimitive = { + Root: RadioGroupPrimitiveRoot, + Item: RadioGroupPrimitiveItem +} as const; -RadioGroupPrimitive.Root = RadioGroupPrimitiveRoot; -RadioGroupPrimitive.Item = RadioGroupPrimitiveItem;src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (1)
13-19: Optimize event handler with useCallback.The
handleOnChangefunction is recreated on every render. Consider memoizing it.+import React, { PropsWithChildren, useCallback, useContext } from 'react'; - const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => { + const handleOnChange = useCallback((event: React.ChangeEvent<HTMLInputElement>) => { setCheckedItem(value); if (typeof onChange === 'function') { onChange(value); } - }; + }, [value, setCheckedItem, onChange]);src/components/ui/RadioGroup/RadioGroup.stories.tsx (3)
Line range hint
5-10: Add TypeScript types and extract options constant.
- Add proper type annotations
- Extract options to a separate constant
-const RadioButton = (args) => { - const options = [ +const LANGUAGE_OPTIONS = [ { id: 'html', value: 'html', label: 'HTML' }, { id: 'css', value: 'css', label: 'CSS' }, { id: 'javascript', value: 'javascript', label: 'JavaScript' } - ]; +] as const; + +type RadioButtonProps = { + // Add props here +}; + +const RadioButton = (args: RadioButtonProps) => {
18-24: Add error boundary and loading state handling.Consider handling edge cases and loading states.
- <RadioGroup.Root defaultChecked={language} items={options} onChange={handleChange} > + <ErrorBoundary fallback={<div>Something went wrong</div>}> + <RadioGroup.Root + defaultChecked={language} + items={LANGUAGE_OPTIONS} + onChange={handleChange} + disabled={isLoading} + > {options.map((option) => ( <RadioGroup.Item key={option.id} value={option.value}> {option.label} </RadioGroup.Item> ))} </RadioGroup.Root> + </ErrorBoundary>
29-36: Enhance story documentation and add variants.The story could benefit from better documentation and additional variants.
export default { title: 'Components/RadioGroup', component: RadioGroup, + parameters: { + docs: { + description: { + component: 'A radio group component that allows users to select one option from a set.' + } + } + }, render: (args) => <RadioButton {...args}/> }; -export const All = {}; -All.args = {}; +export const Default = { + args: { + defaultChecked: 'css' + } +}; + +export const Disabled = { + args: { + defaultChecked: 'css', + disabled: true + } +};src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.js (1)
6-9: Extract duplicate options array to a shared constant.The options array is duplicated between the
RadioButtonandInFormTemplatecomponents. Extract it to improve maintainability.+const LANGUAGE_OPTIONS = [ + { id: 'html', value: 'html', label: 'HTML' }, + { id: 'css', value: 'css', label: 'CSS' }, + { id: 'javascript', value: 'javascript', label: 'JavaScript' } +]; const RadioButton = (args) => { - const options = [ - { id: 'html', value: 'html', label: 'HTML' }, - { id: 'css', value: 'css', label: 'CSS' }, - { id: 'javascript', value: 'javascript', label: 'JavaScript' }]; + const options = LANGUAGE_OPTIONS;Also applies to: 38-41
src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (1)
1-27: Consider adding error boundaries and improving reusability.To enhance the component's robustness and reusability:
- Wrap the component with an error boundary to gracefully handle rendering errors
- Add prop validation for edge cases (e.g., no children, invalid defaultChecked value)
- Consider extracting common radio group logic into a custom hook for reuse
- Add unit tests to verify accessibility features and edge cases
Would you like me to provide an example implementation for any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/components/ui/RadioCards/RadioCards.stories.tsx(1 hunks)src/components/ui/RadioCards/RadioCards.tsx(1 hunks)src/components/ui/RadioCards/context/RadioCardsContext.tsx(1 hunks)src/components/ui/RadioCards/fragments/RadioCardsItem.tsx(1 hunks)src/components/ui/RadioCards/fragments/RadioCardsRoot.tsx(1 hunks)src/components/ui/RadioGroup/RadioGroup.stories.tsx(1 hunks)src/components/ui/RadioGroup/RadioGroup.tsx(1 hunks)src/components/ui/RadioGroup/context/RadioGroupContext.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupItem.tsx(1 hunks)src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx(1 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.js(1 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx(1 hunks)src/core/primitives/RadioGroup/context/RadioGroupContext.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(1 hunks)styles/themes/components/radiocards.scss(1 hunks)styles/themes/default.scss(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/core/primitives/RadioGroup/context/RadioGroupContext.tsx
- src/components/ui/RadioCards/context/RadioCardsContext.tsx
- src/components/ui/RadioCards/RadioCards.tsx
🔇 Additional comments (4)
styles/themes/default.scss (1)
25-25: LGTM! Import statement follows the established pattern.The new import for
radiocards.scssmaintains the alphabetical ordering of component imports.src/components/ui/RadioGroup/RadioGroup.tsx (1)
6-9: Well-structured compound component implementation!The refactoring to a compound component pattern improves flexibility and composition. This addresses the previous feedback about making items dynamic.
src/components/ui/RadioGroup/fragments/RadioGroupRoot.tsx (2)
1-10: LGTM! Imports and constants are well organized.The imports are properly structured and the component name constant follows the project conventions.
27-27: LGTM! Export statement is correct.The default export follows project conventions.
| { id: 'config-2', value: '16-core CPU', label: '16-core CPU' }, | ||
| { id: 'config-3', value: '32-core CPU', label: '32-core CPU' }]; | ||
|
|
||
| const [language, setLanguage] = useState('css'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update initial state to match available options.
The initial state 'css' doesn't match any of the available option values. Consider updating it to one of the CPU configurations.
- const [language, setLanguage] = useState('css');
+ const [language, setLanguage] = useState('8-core CPU');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [language, setLanguage] = useState('css'); | |
| const [language, setLanguage] = useState('8-core CPU'); |
| <RadioCards.Root defaultChecked={language} onChange={handleChange} > | ||
| {options.map((option) => ( | ||
| <RadioCards.Item key={option.id} value={option.value}> | ||
| <div> | ||
| {option.label} | ||
| </div> | ||
| </RadioCards.Item> | ||
| ))} | ||
| </RadioCards.Root> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-label for better accessibility.
The RadioCards group lacks an aria-label for screen readers. Add one to improve accessibility.
- <RadioCards.Root defaultChecked={language} onChange={handleChange} >
+ <RadioCards.Root defaultChecked={language} onChange={handleChange} aria-label="CPU Configuration">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <RadioCards.Root defaultChecked={language} onChange={handleChange} > | |
| {options.map((option) => ( | |
| <RadioCards.Item key={option.id} value={option.value}> | |
| <div> | |
| {option.label} | |
| </div> | |
| </RadioCards.Item> | |
| ))} | |
| </RadioCards.Root> | |
| <RadioCards.Root defaultChecked={language} onChange={handleChange} aria-label="CPU Configuration" > | |
| {options.map((option) => ( | |
| <RadioCards.Item key={option.id} value={option.value}> | |
| <div> | |
| {option.label} | |
| </div> | |
| </RadioCards.Item> | |
| ))} | |
| </RadioCards.Root> |
| type RadioGroupItemProps = PropsWithChildren<{ | ||
| value: string; | ||
| }>; |
There was a problem hiding this comment.
Fix type naming inconsistency.
The type is named RadioGroupItemProps but used as RadioGroupPrimitiveItemProps in the component.
-type RadioGroupItemProps = PropsWithChildren<{
+type RadioGroupPrimitiveItemProps = PropsWithChildren<{
value: string;
}>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type RadioGroupItemProps = PropsWithChildren<{ | |
| value: string; | |
| }>; | |
| type RadioGroupPrimitiveItemProps = PropsWithChildren<{ | |
| value: string; | |
| }>; |
| return ( | ||
| <div {...props}> | ||
| <RadioPrimitive value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} /> | ||
| <label htmlFor={value}>{children}</label> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and label association.
The current implementation has accessibility issues:
- Missing aria attributes
- Label not properly associated with input
- Missing role attribute
return (
- <div {...props}>
+ <div role="radio" aria-checked={checkedItem === value} {...props}>
<RadioPrimitive
value={value}
name='radio'
+ id={`radio-${value}`}
checked={checkedItem === value}
onChange={handleOnChange}
/>
- <label htmlFor={value}>{children}</label>
+ <label htmlFor={`radio-${value}`}>{children}</label>
</div>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div {...props}> | |
| <RadioPrimitive value={value} name='radio' checked={checkedItem === value} onChange={handleOnChange} /> | |
| <label htmlFor={value}>{children}</label> | |
| </div> | |
| ); | |
| return ( | |
| <div role="radio" aria-checked={checkedItem === value} {...props}> | |
| <RadioPrimitive | |
| value={value} | |
| name='radio' | |
| id={`radio-${value}`} | |
| checked={checkedItem === value} | |
| onChange={handleOnChange} | |
| /> | |
| <label htmlFor={`radio-${value}`}>{children}</label> | |
| </div> | |
| ); |
| type RadioGroupRootProps = PropsWithChildren<{ | ||
| className?: string; | ||
| customRootClass?: string; | ||
| defaultChecked?: string; | ||
| onChange?: (item: string) => void; | ||
| }>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused prop and add controlled mode support.
- The
customRootClassprop is defined but never used - Consider adding support for controlled mode
type RadioGroupRootProps = PropsWithChildren<{
className?: string;
- customRootClass?: string;
defaultChecked?: string;
+ value?: string;
onChange?: (item: string) => void;
}>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type RadioGroupRootProps = PropsWithChildren<{ | |
| className?: string; | |
| customRootClass?: string; | |
| defaultChecked?: string; | |
| onChange?: (item: string) => void; | |
| }>; | |
| type RadioGroupRootProps = PropsWithChildren<{ | |
| className?: string; | |
| defaultChecked?: string; | |
| value?: string; | |
| onChange?: (item: string) => void; | |
| }>; |
| return <RadioGroupContext.Provider value={sendItems}> | ||
| <div {...props}>{children}</div> | ||
| </RadioGroupContext.Provider>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper ARIA attributes for accessibility.
The root element should have proper role and aria attributes.
return <RadioGroupContext.Provider value={sendItems}>
- <div {...props}>{children}</div>
+ <div
+ role="radiogroup"
+ aria-required="false"
+ aria-orientation="vertical"
+ {...props}
+ >
+ {children}
+ </div>
</RadioGroupContext.Provider>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return <RadioGroupContext.Provider value={sendItems}> | |
| <div {...props}>{children}</div> | |
| </RadioGroupContext.Provider>; | |
| return <RadioGroupContext.Provider value={sendItems}> | |
| <div | |
| role="radiogroup" | |
| aria-required="false" | |
| aria-orientation="vertical" | |
| {...props} | |
| > | |
| {children} | |
| </div> | |
| </RadioGroupContext.Provider>; |
| const RadioGroupPrimitiveRoot = ({ children, defaultChecked = '', onChange = null, ...props }: RadioGroupPrimitiveRootProps) => { | ||
| const [checkedItem, setCheckedItem] = useState(defaultChecked); | ||
|
|
||
| const handleOnChange = (item: string) => { | ||
| setCheckedItem(item); | ||
|
|
||
| if (typeof onChange === 'function') { | ||
| onChange(item); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add controlled mode support and optimize onChange handler.
The component should support both controlled and uncontrolled modes.
-const RadioGroupPrimitiveRoot = ({ children, defaultChecked = '', onChange = null, ...props }: RadioGroupPrimitiveRootProps) => {
+const RadioGroupPrimitiveRoot = ({
+ children,
+ defaultChecked = '',
+ value: controlledValue,
+ onChange = null,
+ ...props
+}: RadioGroupPrimitiveRootProps) => {
const [checkedItem, setCheckedItem] = useState(defaultChecked);
+ const isControlled = controlledValue !== undefined;
+ const currentValue = isControlled ? controlledValue : checkedItem;
- const handleOnChange = (item: string) => {
- setCheckedItem(item);
+ const handleOnChange = useCallback((item: string) => {
+ if (!isControlled) {
+ setCheckedItem(item);
+ }
if (typeof onChange === 'function') {
onChange(item);
}
- };
+ }, [isControlled, onChange]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const RadioGroupPrimitiveRoot = ({ children, defaultChecked = '', onChange = null, ...props }: RadioGroupPrimitiveRootProps) => { | |
| const [checkedItem, setCheckedItem] = useState(defaultChecked); | |
| const handleOnChange = (item: string) => { | |
| setCheckedItem(item); | |
| if (typeof onChange === 'function') { | |
| onChange(item); | |
| } | |
| }; | |
| const RadioGroupPrimitiveRoot = ({ | |
| children, | |
| defaultChecked = '', | |
| value: controlledValue, | |
| onChange = null, | |
| ...props | |
| }: RadioGroupPrimitiveRootProps) => { | |
| const [checkedItem, setCheckedItem] = useState(defaultChecked); | |
| const isControlled = controlledValue !== undefined; | |
| const currentValue = isControlled ? controlledValue : checkedItem; | |
| const handleOnChange = useCallback((item: string) => { | |
| if (!isControlled) { | |
| setCheckedItem(item); | |
| } | |
| if (typeof onChange === 'function') { | |
| onChange(item); | |
| } | |
| }, [isControlled, onChange]); |
| const handleChange = (e) => { | ||
| setLanguage(e.target.value); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize event handling between stories.
The event handling is inconsistent between the two stories. RadioButton uses e.target.value while InFormTemplate uses the direct value.
const RadioButton = (args) => {
- const handleChange = (e) => {
- setLanguage(e.target.value);
+ const handleChange = (value) => {
+ setLanguage(value);
};Also applies to: 45-48
| <RadioGroupPrimitive.Root defaultChecked={language} items={options} onChange={handleChange} > | ||
| {options.map((option) => ( | ||
| <RadioGroupPrimitive.Item key={option.id} value={option.value}> | ||
| {option.label} | ||
| </RadioGroupPrimitive.Item> | ||
| ))} | ||
| </RadioGroupPrimitive.Root> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA labels for better accessibility.
The radio group needs an accessible label and description.
<RadioGroupPrimitive.Root
defaultChecked={language}
items={options}
- onChange={handleChange}>
+ onChange={handleChange}
+ aria-label="Programming Language"
+ aria-description="Select your preferred programming language">Also applies to: 58-64
| export const RadioGroupContext = createContext({ | ||
| defaultChecked: null, | ||
| customRootClass: null, | ||
| onChange: null | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript types for context values.
The context values lack proper TypeScript types, which could lead to type safety issues.
+type RadioGroupContextType = {
+ defaultChecked: string | null;
+ customRootClass: string | null;
+ onChange: ((value: string) => void) | null;
+};
-export const RadioGroupContext = createContext({
+export const RadioGroupContext = createContext<RadioGroupContextType>({
defaultChecked: null,
customRootClass: null,
onChange: null
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const RadioGroupContext = createContext({ | |
| defaultChecked: null, | |
| customRootClass: null, | |
| onChange: null | |
| }); | |
| type RadioGroupContextType = { | |
| defaultChecked: string | null; | |
| customRootClass: string | null; | |
| onChange: ((value: string) => void) | null; | |
| }; | |
| export const RadioGroupContext = createContext<RadioGroupContextType>({ | |
| defaultChecked: null, | |
| customRootClass: null, | |
| onChange: null | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/ui/AspectRatio/stories/AspectRatio.stories.tsx (1)
1-1: Consider removing the explicit JSX import.While consolidating React imports is good practice, the explicit JSX import appears unnecessary as it's not directly used in the code. The JSX syntax transformation is handled by the TypeScript compiler regardless of whether JSX is explicitly imported.
-import React, { JSX } from 'react'; +import React from 'react';🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'JSX' is defined but never usedsrc/components/ui/Switch/tests/Switch.test.tsx (1)
41-41: Enhance test coverage with additional test cases.Consider adding tests for:
- Disabled state behavior
- Keyboard interactions (Space/Enter keys)
- Label association and accessibility
Here's an example of additional test cases to add:
test('handles disabled state correctly', () => { render(<Switch disabled checked={false} onChange={() => {}} />); const switchElement = screen.getByRole('switch'); expect(switchElement).toBeDisabled(); fireEvent.click(switchElement); expect(switchElement).not.toBeChecked(); }); test('responds to keyboard interactions', () => { const handleChange = jest.fn(); render(<Switch checked={false} onChange={handleChange} />); const switchElement = screen.getByRole('switch'); switchElement.focus(); fireEvent.keyDown(switchElement, { key: 'Enter' }); expect(handleChange).toHaveBeenCalledWith(true); fireEvent.keyDown(switchElement, { key: ' ' }); expect(handleChange).toHaveBeenCalledWith(false); }); test('associates with label correctly', () => { render( <label> <Switch checked={false} onChange={() => {}} /> Toggle me </label> ); const switchElement = screen.getByRole('switch'); const label = screen.getByText('Toggle me'); expect(switchElement).toHaveAccessibleName('Toggle me'); });src/components/ui/RadioCards/RadioCards.stories.tsx (3)
5-9: Add TypeScript interfaces for better type safety.Consider adding TypeScript interfaces to define the shape of your options and props:
interface RadioOption { id: string; value: string; label: string; } interface RadioCardsTemplateProps { args: Record<string, unknown>; }
11-15: Rename state variable to match context.The variable name 'language' doesn't match the context of CPU configurations. Consider renaming for clarity:
- const [language, setLanguage] = useState('css'); - const handleChange = (e) => { - setLanguage(e.target.value); + const [cpuConfig, setCpuConfig] = useState('8-core CPU'); + const handleChange = (e) => { + setCpuConfig(e.target.value);
38-39: Document available story args.Consider adding documentation for the story's arguments, even if currently empty:
export const All = { parameters: { docs: { description: { story: 'Basic usage of RadioCards component with CPU configuration options.', }, }, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.eslintrc.cjs(1 hunks)src/components/ui/AspectRatio/stories/AspectRatio.stories.tsx(1 hunks)src/components/ui/Avatar/stories/Avatar.stories.tsx(2 hunks)src/components/ui/AvatarGroup/stories/AvatarGroup.stories.tsx(3 hunks)src/components/ui/BlockQuote/stories/BlockQuote.stories.tsx(2 hunks)src/components/ui/Button/stories/Button.stories.tsx(1 hunks)src/components/ui/Callout/stories/Callout.stories.tsx(1 hunks)src/components/ui/Code/stories/Code.stories.tsx(2 hunks)src/components/ui/Disclosure/stories/Disclosure.stories.tsx(1 hunks)src/components/ui/Dropdown/stories/Dropdown.stories.tsx(1 hunks)src/components/ui/HoverCard/stories/HoverCard.stories.js(1 hunks)src/components/ui/RadioCards/RadioCards.stories.tsx(1 hunks)src/components/ui/RadioGroup/RadioGroup.stories.tsx(1 hunks)src/components/ui/Separator/Separator.tsx(1 hunks)src/components/ui/Separator/stories/Separator.stories.js(1 hunks)src/components/ui/Switch/Switch.tsx(2 hunks)src/components/ui/Switch/stories/Switch.stories.js(2 hunks)src/components/ui/Switch/tests/Switch.test.tsx(1 hunks)src/components/ui/Toggle/Toggle.tsx(2 hunks)src/components/ui/Toggle/stories/Toggle.stories.js(1 hunks)src/components/ui/Toggle/tests/Toggle.test.js(1 hunks)src/components/ui/ToggleGroup/ToggleGroup.tsx(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx(2 hunks)src/components/ui/ToggleGroup/stories/ToggleGroup.stories.js(1 hunks)src/components/ui/ToggleGroup/tests/ToggleGroup.test.js(3 hunks)
✅ Files skipped from review due to trivial changes (16)
- src/components/ui/Code/stories/Code.stories.tsx
- src/components/ui/Separator/stories/Separator.stories.js
- src/components/ui/Switch/Switch.tsx
- src/components/ui/ToggleGroup/stories/ToggleGroup.stories.js
- src/components/ui/Dropdown/stories/Dropdown.stories.tsx
- src/components/ui/Separator/Separator.tsx
- src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx
- src/components/ui/Button/stories/Button.stories.tsx
- src/components/ui/Callout/stories/Callout.stories.tsx
- src/components/ui/BlockQuote/stories/BlockQuote.stories.tsx
- src/components/ui/Avatar/stories/Avatar.stories.tsx
- src/components/ui/Toggle/Toggle.tsx
- src/components/ui/HoverCard/stories/HoverCard.stories.js
- src/components/ui/ToggleGroup/ToggleGroup.tsx
- src/components/ui/AvatarGroup/stories/AvatarGroup.stories.tsx
- src/components/ui/Disclosure/stories/Disclosure.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/components/ui/Toggle/tests/Toggle.test.js
- .eslintrc.cjs
- src/components/ui/Switch/stories/Switch.stories.js
- src/components/ui/Toggle/stories/Toggle.stories.js
- src/components/ui/ToggleGroup/tests/ToggleGroup.test.js
- src/components/ui/RadioGroup/RadioGroup.stories.tsx
🧰 Additional context used
🪛 GitHub Check: lint
src/components/ui/AspectRatio/stories/AspectRatio.stories.tsx
[warning] 1-1:
'JSX' is defined but never used
🔇 Additional comments (5)
src/components/ui/AspectRatio/stories/AspectRatio.stories.tsx (2)
Line range hint
8-19: LGTM! Well-structured Storybook story.The story follows Storybook's best practices:
- Uses the recommended default export format
- Properly types the render function arguments
- Includes helpful comments referencing Storybook documentation
🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'JSX' is defined but never used
[warning] 2-2:
'AspectRatioProps' is defined but never used
Line range hint
22-27: LGTM! Clear and concise story configuration.The Default story is well-structured with appropriate default props.
🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'JSX' is defined but never used
[warning] 2-2:
'AspectRatioProps' is defined but never usedsrc/components/ui/Switch/tests/Switch.test.tsx (1)
1-3: LGTM! Clean and well-structured imports.The import statements are properly organized and follow consistent formatting.
src/components/ui/RadioCards/RadioCards.stories.tsx (2)
11-11: Update initial state to match available options.The initial state 'css' doesn't match any of the available option values. Consider updating it to one of the CPU configurations.
- const [language, setLanguage] = useState('css'); + const [language, setLanguage] = useState('8-core CPU');
19-27: Add aria-label for better accessibility.The RadioCards group lacks an aria-label for screen readers. Add one to improve accessibility.
- <RadioCards.Root defaultChecked={language} onChange={handleChange} > + <RadioCards.Root defaultChecked={language} onChange={handleChange} aria-label="CPU Configuration">
| }) | ||
| render(<Switch checked={true} onChange={() => {}} />); | ||
| const inputElement = screen.getByRole('checkbox'); |
There was a problem hiding this comment.
Standardize ARIA roles across tests.
There's an inconsistency in the ARIA role usage across test cases:
- Lines 8, 26, 32 use
role='checkbox' - Line 15 uses
role='switch'
According to WAI-ARIA practices, a switch component should consistently use role='switch' to properly convey its semantics to assistive technologies.
Apply this change across all test cases:
-const inputElement = screen.getByRole('checkbox');
+const inputElement = screen.getByRole('switch');Also applies to: 15-15, 26-26, 32-32
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Code Formatting
Component Updates
RadioCards,RadioGroup, andRadioGroupPrimitivecomponentsSwitch,Toggle, andToggleGroupcomponentsStorybook Organization
Context and Primitive Components
Styling