-
-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Toggle Primitive, Toggle, ToggleGroup Refactor #576
Conversation
WalkthroughThe pull request introduces several changes across multiple files. Key updates include modifications to Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/core/primitives/Toggle/tests/TogglePrimitive.test.jsOops! Something went wrong! :( ESLint: 8.56.0 Error: Failed to load parser '@babel/eslint-parser' declared in '.eslintrc.cjs': Cannot find module '@babel/core/package.json'
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #576 +/- ##
==========================================
+ Coverage 92.10% 94.00% +1.89%
==========================================
Files 13 14 +1
Lines 76 100 +24
Branches 16 27 +11
==========================================
+ Hits 70 94 +24
Misses 6 6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx (3)
8-15
: Consider reordering props to prevent potential overrides.The spread operator
{...args}
before specific props could allow story arguments to override essential functionality.Consider this ordering:
-<TogglePrimitive {...args} pressed={pressed} onPressedChange={setPressed}> +<TogglePrimitive pressed={pressed} onPressedChange={setPressed} {...args}>
12-12
: Enhance toggle state text for better accessibility.The current state text "on/off" could be more descriptive for better user understanding.
Consider something more descriptive:
-toggle - {pressed ? 'on' : 'off'} +Toggle is {pressed ? 'enabled' : 'disabled'}
17-22
: Enhance story with additional props and documentation.The story could better demonstrate the component's capabilities by including more props and documentation.
Consider adding:
- Different variants/states
- Documentation about available props
- Examples of common use cases
export const All = { + parameters: { + docs: { + description: { + component: 'A primitive toggle component that handles press state.', + }, + }, + }, args: { - className: '' + className: '', + disabled: false, + 'aria-label': 'Toggle example' } };src/components/ui/Toggle/Toggle.tsx (3)
30-32
: Consider removing redundant runtime type checkSince TypeScript already provides compile-time type checking, this runtime check might be unnecessary. If you need to maintain runtime type checking for JavaScript users, consider using a validation library like Zod or io-ts for more comprehensive prop validation.
Line range hint
36-42
: Resolve controlled/uncontrolled component ambiguityThe component accepts both
pressed
prop and maintains internal state, which could lead to synchronization issues. It should either be fully controlled or uncontrolled, not both.Consider this pattern instead:
const handlePressed = () => { const updatedPressed = !pressed; onChange(updatedPressed); };And remove the internal state if the component should be controlled, or use defaultPressed for uncontrolled mode.
46-54
: Consider implementing compound components patternThe current implementation could benefit from a compound components pattern, which would provide more flexibility and better separation of concerns.
Example structure:
// Usage <Toggle> <Toggle.Button> <Toggle.Icon /> <Toggle.Label>Label</Toggle.Label> </Toggle.Button> </Toggle>This would allow for more flexible styling and composition while maintaining the primitive-based approach.
src/core/primitives/Toggle/index.tsx (2)
6-6
: Simplify the type annotation fordefaultPressed
.The type
boolean | false
is redundant becauseboolean
already includesfalse
. It should simply beboolean
.Apply this diff to correct the type annotation:
- defaultPressed? : boolean | false; + defaultPressed?: boolean;
23-24
: Specify the type instead of usingany
forariaAttributes
.Using
any
defeats the purpose of TypeScript's type checking. Providing a specific type enhances code safety and readability.Apply this diff to define a proper type:
- const ariaAttributes:any = label ? { 'aria-label': label } : {}; + const ariaAttributes: { [key: string]: string } = label ? { 'aria-label': label } : {};Alternatively, use TypeScript’s built-in types for ARIA attributes if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
package.json
(2 hunks)src/components/ui/Toggle/Toggle.tsx
(3 hunks)src/components/ui/ToggleGroup/fragments/ToggleItem.tsx
(0 hunks)src/core/primitives/Toggle/contexts/TogglePrimitiveContext.tsx
(0 hunks)src/core/primitives/Toggle/fragments/TogglePrimitiveRoot.tsx
(0 hunks)src/core/primitives/Toggle/index.tsx
(1 hunks)src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx
(1 hunks)src/core/primitives/Toggle/tests/TogglePrimitive.test.js
(1 hunks)
💤 Files with no reviewable changes (3)
- src/components/ui/ToggleGroup/fragments/ToggleItem.tsx
- src/core/primitives/Toggle/contexts/TogglePrimitiveContext.tsx
- src/core/primitives/Toggle/fragments/TogglePrimitiveRoot.tsx
🔇 Additional comments (5)
src/core/primitives/Toggle/tests/TogglePrimitive.test.js (2)
1-3
: LGTM! Import statements are clean and necessary.
The imports are appropriate for the testing requirements.
5-10
:
Test coverage needs significant expansion.
The current test only verifies basic rendering, but according to the PR summary, TogglePrimitive has been transformed into a functional component with state management and accessibility features. Consider adding tests for:
- State management
- Initial state
- State changes through interactions
- Accessibility features
- ARIA attributes
- Keyboard navigation
- Interactive behavior
- Click handlers
- Toggle functionality
Here's a suggested expansion of the test suite:
describe('TogglePrimitive', () => {
it('renders children correctly', () => {
render(<TogglePrimitive>Test Content</TogglePrimitive>);
expect(screen.getByText('Test Content')).toBeInTheDocument();
});
+
+ it('manages toggle state correctly', () => {
+ render(<TogglePrimitive>Toggle</TogglePrimitive>);
+ const toggle = screen.getByText('Toggle');
+
+ expect(toggle).not.toHaveAttribute('data-state', 'on');
+ userEvent.click(toggle);
+ expect(toggle).toHaveAttribute('data-state', 'on');
+ });
+
+ it('supports keyboard interaction', () => {
+ render(<TogglePrimitive>Toggle</TogglePrimitive>);
+ const toggle = screen.getByText('Toggle');
+
+ toggle.focus();
+ expect(document.activeElement).toBe(toggle);
+
+ fireEvent.keyDown(toggle, { key: 'Enter' });
+ expect(toggle).toHaveAttribute('data-state', 'on');
+ });
+
+ it('has correct accessibility attributes', () => {
+ render(<TogglePrimitive>Toggle</TogglePrimitive>);
+ const toggle = screen.getByText('Toggle');
+
+ expect(toggle).toHaveAttribute('role', 'button');
+ expect(toggle).toHaveAttribute('aria-pressed', 'false');
+ });
});
Let's verify the component's implementation to ensure our test suggestions align with the actual features:
src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx (1)
1-3
: LGTM! Import statements are well-structured.
The imports are correctly organized and include all necessary dependencies for the component's functionality.
package.json (2)
11-11
: Verify new component files exist in the dist directory.
The new exports follow the established pattern and maintain alphabetical order. Let's verify that these components are properly built and available in the dist directory.
Also applies to: 13-13, 27-27
✅ Verification successful
All new components exist and are properly structured
The verification confirms that:
- All three components (
AlertDialog
,AvatarGroup
,RadioGroup
) exist in the source directory undersrc/components/ui/
- Each component follows proper import patterns and component structure
- The components are organized in a modular way with their fragments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new component files exist in the dist directory
# and check their imports to ensure they're properly structured
# Check if the component files exist in the source
fd -t f "^(AlertDialog|AvatarGroup|RadioGroup)\.(tsx|jsx)$" src/
# Check their imports to ensure they're properly structured
for component in AlertDialog AvatarGroup RadioGroup; do
echo "Checking imports for $component:"
rg -A 5 "^import.*from" $(fd -t f "^${component}\.(tsx|jsx)$" src/)
done
Length of output: 2290
11-11
: Verify component relationships with Toggle refactoring.
Since this PR focuses on Toggle primitive refactoring, let's understand how these new components relate to or depend on the Toggle components.
Also applies to: 13-13, 27-27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx (2)
8-15
: Consider adding proper TypeScript types for story args.The implementation looks good, but using
any
type for args reduces type safety. Consider defining proper types based on TogglePrimitive's props interface.- render: (args:any) => { + render: (args: React.ComponentProps<typeof TogglePrimitive>) => {
17-28
: Enhance story variants to better showcase component capabilities.Consider the following improvements:
- Remove the empty className from All variant as it adds no value
- Add more variants to demonstrate different use cases (e.g., custom styling, different sizes, with icons)
- Add documentation about the component's props and usage
export const All = { - args: { - className: '' - } + args: {} };src/core/primitives/Toggle/index.tsx (1)
24-25
: Improve type safety for ARIA attributesUsing
any
type for ARIA attributes reduces type safety.Consider using proper typing:
- const ariaAttributes:any = label ? { 'aria-label': label } : {}; + const ariaAttributes: { + 'aria-label'?: string; + 'aria-pressed': 'true' | 'false'; + } = { + ...(label ? { 'aria-label': label } : {}), + 'aria-pressed': isPressed ? 'true' : 'false' + }; - ariaAttributes['aria-pressed'] = isPressed ? 'true' : 'false';src/core/primitives/Toggle/tests/TogglePrimitive.test.js (1)
35-40
: Enhance event handling test coverageThe current test verifies basic callback functionality, but we should also test:
- Callback not triggered when disabled
- Multiple interactions tracking correct boolean states
it('does not trigger onPressedChange when disabled', () => { const onPressedChange = jest.fn(); render( <TogglePrimitive disabled onPressedChange={onPressedChange}> Test Content </TogglePrimitive> ); fireEvent.click(screen.getByRole('button')); expect(onPressedChange).not.toHaveBeenCalled(); }); it('tracks multiple interactions correctly', () => { const onPressedChange = jest.fn(); render(<TogglePrimitive onPressedChange={onPressedChange}>Test Content</TogglePrimitive>); const button = screen.getByRole('button'); fireEvent.click(button); expect(onPressedChange).toHaveBeenNthCalledWith(1, true); fireEvent.click(button); expect(onPressedChange).toHaveBeenNthCalledWith(2, false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/core/primitives/Toggle/index.tsx
(1 hunks)src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx
(1 hunks)src/core/primitives/Toggle/tests/TogglePrimitive.test.js
(1 hunks)
🔇 Additional comments (5)
src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx (2)
1-3
: LGTM! Imports are appropriate and well-structured.
1-28
: Verify integration with Toggle component changes.
Let's verify that this story properly demonstrates the new primitive approach and integrates well with the refactored Toggle component.
✅ Verification successful
Integration with Toggle component changes is properly implemented
The story correctly demonstrates the new primitive approach with consistent prop usage:
- The story uses the same
pressed
andonPressedChange
props that are defined in bothTogglePrimitive
andToggle
components - The primitive component properly handles state management and prop passing
- The story demonstrates the toggle state visually with "on/off" text
- Both disabled and default states are covered in the story examples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent prop usage across Toggle components
echo "Checking for consistent prop usage..."
rg -A 2 "onPressedChange|pressed" src/core/primitives/Toggle/
rg -A 2 "onPressedChange|pressed" src/components/ui/Toggle/
# Verify primitive approach implementation
echo "Verifying primitive implementation..."
ast-grep --pattern 'export default function TogglePrimitive' src/core/primitives/Toggle/
Length of output: 4032
src/core/primitives/Toggle/index.tsx (2)
27-34
: LGTM! Render implementation follows best practices
The render logic correctly:
- Uses semantic button element via Primitive
- Implements proper props spreading order
- Provides styling hooks via data attributes
1-1
: 🛠️ Refactor suggestion
Add missing useEffect import
The useEffect hook will be needed for state synchronization.
-import React, { useState } from 'react';
+import React, { useState, useEffect } from 'react';
Likely invalid or redundant comment.
src/core/primitives/Toggle/tests/TogglePrimitive.test.js (1)
1-3
: LGTM! Imports are properly structured.
The necessary testing utilities and component imports are correctly set up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
styles/themes/components/toggle.scss (1)
22-24
: Great accessibility improvement!The switch from
:focus
to:focus-visible
with outline styling is an excellent change that:
- Shows focus indicators only for keyboard navigation
- Maintains clean visual design for mouse users
- Provides clear visual feedback with the outline and offset
Consider documenting these accessibility features in your component documentation to help other developers understand the importance of maintaining these focus styles.
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (3)
10-14
: Add proper typing for the refThe ref lacks proper TypeScript typing which could lead to runtime errors.
-const toggleGroupRef = useRef(null); +const toggleGroupRef = useRef<HTMLDivElement | null>(null);
16-30
: Enhance navigation functions with error handling and accessibilityWhile the navigation functions work, they could benefit from better error handling and accessibility improvements.
- Add error handling:
const nextItem = () => { + if (!toggleGroupRef?.current) { + console.warn('ToggleGroup: No group element found'); + return; + } const batches = getAllBatchElements(toggleGroupRef?.current); + if (!batches.length) { + console.warn('ToggleGroup: No toggle items found'); + return; + } const nextItem = getNextBatchItem(batches, loop); if (nextItem) { nextItem?.focus(); } };
- Consider adding ARIA attributes to improve accessibility:
-<div className={`${rootClass} ${className}`} role="group" ref={toggleGroupRef}> +<div + className={`${rootClass} ${className}`} + role="group" + ref={toggleGroupRef} + aria-orientation="horizontal" + aria-label="Toggle group">
32-38
: Optimize performance with useMemo for context valueThe context value object is recreated on every render, which could cause unnecessary rerenders in child components.
+import React, { useState, useRef, useMemo } from 'react'; -const sendValues = { +const sendValues = useMemo(() => ({ nextItem, previousItem, activeToggles, setActiveToggles, type -}; +}), [nextItem, previousItem, activeToggles, type]);src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (6)
13-14
: Ensure context values are properly defined and typedDestructure
ToggleContext
with precise typings to avoid potential runtime errors and improve code maintainability.Consider specifying the types for the context values:
const { type, activeToggles, setActiveToggles, nextItem, previousItem } = useContext(ToggleContext); +// Ensure ToggleContext provides the correct types for these values
18-19
: Specify types forariaProps
anddataProps
Defining the types explicitly enhances readability and type safety.
You can define the objects as follows:
-const ariaProps: any = {}; -const dataProps: any = {}; +const ariaProps: { [key: string]: string } = {}; +const dataProps: { [key: string]: string } = {};
25-27
: Type event handlers explicitlySpecify the event types for
handleBlur
and similar functions to improve code clarity and type checking.Example:
-const handleBlur = () => { +const handleBlur = (event: React.FocusEvent<HTMLButtonElement>) => { setIsFocused(false); };Apply similar updates to other event handlers like
handleFocus
,handleKeyDown
, andhandleToggleSelect
if applicable.
55-65
: Type thehandleKeyDown
event parameter properlyTyping the event parameter improves type safety and leverages TypeScript's features.
Update the function signature:
-const handleKeyDown = (e: any) => { +const handleKeyDown = (e: React.KeyboardEvent<HTMLButtonElement>) => {
69-73
: Use boolean values for ARIA attributesWhile React accepts strings for ARIA attributes, using boolean values where appropriate enhances code readability.
Consider setting
aria-pressed
with boolean values:if (isActive) { - ariaProps['aria-pressed'] = 'true'; + ariaProps['aria-pressed'] = true; dataProps['data-active'] = 'true'; } else { - ariaProps['aria-pressed'] = 'false'; + ariaProps['aria-pressed'] = false; dataProps['data-active'] = 'false'; }
76-87
: Simplify the rendering of conditional data attributesStreamline the logic for applying conditional attributes to improve code readability.
You can simplify the spread of
isFocused
data attribute:return <TogglePrimitive onClick={handleToggleSelect} onFocus={handleFocus} onBlur={handleBlur} {...ariaProps} {...dataProps} data-rad-ui-batch-element onKeyDown={handleKeyDown} - {...isFocused ? { 'data-rad-ui-focus-element': '' } : {}} + data-rad-ui-focus-element={isFocused ? '' : undefined} {...props} > {children} </TogglePrimitive>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/components/ui/Accordion/fragments/AccordionItem.tsx
(0 hunks)src/components/ui/Toggle/Toggle.tsx
(3 hunks)src/components/ui/ToggleGroup/contexts/toggleContext.tsx
(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx
(1 hunks)src/components/ui/ToggleGroup/fragments/ToggleItem.tsx
(2 hunks)src/core/batches.ts
(1 hunks)styles/themes/components/toggle-group.scss
(2 hunks)styles/themes/components/toggle.scss
(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/Accordion/fragments/AccordionItem.tsx
🔇 Additional comments (5)
styles/themes/components/toggle-group.scss (3)
3-3
: Verify touch target accessibility with reduced gap
The reduction in gap spacing from 12px to 6px might affect touch target accessibility. With 35x35px buttons and 6px gaps, we should verify that the touch targets remain easily clickable on mobile devices.
Consider maintaining a minimum touch target area of 44x44px (as per WCAG guidelines) by either:
- Increasing the button size, or
- Adding padding to the clickable area while maintaining visual size
18-18
: LGTM: Good use of design system variables
The addition of a border using the design system's color variable (--rad-ui-color-accent-300
) improves button visibility while maintaining consistency with the design system.
20-22
: Consider contrast ratio for focus outline
The switch to :focus-visible
is a good modernization, but with the reduced outline-offset (2px) and potentially low-contrast outline color, we should verify the focus state visibility.
Consider adding a higher contrast focus state:
&:focus-visible {
- outline: 2px solid var(--rad-ui-color-accent-900);
+ outline: 2px solid var(--rad-ui-color-accent-900);
+ box-shadow: 0 0 0 2px var(--rad-ui-color-accent-100);
outline-offset: 2px;
}
src/components/ui/ToggleGroup/fragments/ToggleGroupRoot.tsx (1)
4-4
: Verify batch utility functions implementation and usage
The component relies heavily on newly imported batch utility functions. Let's verify their implementation and test coverage.
src/components/ui/Toggle/Toggle.tsx (1)
10-11
: Align type definition with component implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/core/primitives/Toggle/index.tsx (1)
41-43
: Improve type safety for ARIA attributesUsing
any
type reduces type safety. Consider using proper types from React's type definitions.const ariaAttributes: React.AriaAttributes = { ...(label ? { 'aria-label': label } : {}), 'aria-pressed': isPressed ? 'true' : 'false', 'aria-disabled': disabled ? 'true' : 'false' };src/core/primitives/Toggle/tests/TogglePrimitive.test.js (3)
11-21
: Enhance asChild and label tests for better coverageWhile the basic functionality is tested, consider strengthening these tests:
For asChild:
- Verify that original button props are preserved
- Check if event handlers are properly forwarded
For label:
- Directly verify the aria-label attribute instead of using getByLabelText
it('renders asChild correctly', () => { + const onClick = jest.fn(); const { container } = render( - <TogglePrimitive asChild><button>Click me</button></TogglePrimitive> + <TogglePrimitive asChild> + <button onClick={onClick} className="original-class">Click me</button> + </TogglePrimitive> ); - expect(container.querySelector('button')).toBeInTheDocument(); - expect(container.querySelector('button')).toHaveTextContent('Click me'); + const button = container.querySelector('button'); + expect(button).toBeInTheDocument(); + expect(button).toHaveTextContent('Click me'); + expect(button).toHaveClass('original-class'); + fireEvent.click(button); + expect(onClick).toHaveBeenCalled(); }); it('renders with label correctly', () => { render(<TogglePrimitive label="Test Label">Test Content</TogglePrimitive>); expect(screen.getByText('Test Content')).toBeInTheDocument(); - expect(screen.getByLabelText('Test Label')).toBeInTheDocument(); + expect(screen.getByRole('button')).toHaveAttribute('aria-label', 'Test Label'); });
23-33
: Add test for default state and enhance pressed state verificationThe current tests should be expanded to:
- Test the default state when defaultPressed is undefined
- Verify both data-state and aria-pressed attributes
+it('renders with default state correctly when defaultPressed is undefined', () => { + render(<TogglePrimitive>Test Content</TogglePrimitive>); + const button = screen.getByRole('button'); + expect(button).toHaveAttribute('data-state', 'off'); + expect(button).toHaveAttribute('aria-pressed', 'false'); +}); it('renders with defaultPressed true with data-state on correctly', () => { render(<TogglePrimitive defaultPressed={true}>Test Content</TogglePrimitive>); - expect(screen.getByText('Test Content')).toBeInTheDocument(); - expect(screen.getByRole('button')).toHaveAttribute('data-state', 'on'); + const button = screen.getByRole('button'); + expect(button).toHaveAttribute('data-state', 'on'); + expect(button).toHaveAttribute('aria-pressed', 'true'); });
66-87
: Add edge cases to controlled mode testsConsider adding tests for these scenarios:
- Switching between controlled and uncontrolled modes
- Handling undefined pressed prop in controlled mode
- Warning when pressed is provided without onPressedChange
it('warns when pressed is provided without onPressedChange', () => { const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); render(<TogglePrimitive pressed={false}>Test Content</TogglePrimitive>); expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining('You provided a `pressed` prop without an `onPressedChange` handler') ); consoleSpy.mockRestore(); }); it('handles switching between controlled and uncontrolled modes', () => { const { rerender } = render(<TogglePrimitive defaultPressed={true}>Test Content</TogglePrimitive>); expect(screen.getByRole('button')).toHaveAttribute('data-state', 'on'); // Switch to controlled mode rerender(<TogglePrimitive pressed={false}>Test Content</TogglePrimitive>); expect(screen.getByRole('button')).toHaveAttribute('data-state', 'off'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/core/primitives/Toggle/index.tsx
(1 hunks)src/core/primitives/Toggle/tests/TogglePrimitive.test.js
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/core/primitives/Toggle/index.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
🪛 GitHub Check: codecov/patch
src/core/primitives/Toggle/index.tsx
[warning] 31-31: src/core/primitives/Toggle/index.tsx#L31
Added line #L31 was not covered by tests
🔇 Additional comments (5)
src/core/primitives/Toggle/index.tsx (5)
15-23
: LGTM! Clean props destructuring with meaningful defaults
The props destructuring is well-organized with appropriate default values, and the renaming of pressed
to controlledPressed
improves code clarity.
24-28
: LGTM! Well-implemented state management pattern
The state management implementation properly handles both controlled and uncontrolled modes, with clear separation of concerns.
45-52
: LGTM! Well-structured component rendering
The component rendering follows best practices:
- Uses semantic button element
- Correct props spreading order
- Provides state information for styling
5-13
: 🛠️ Refactor suggestion
Improve API consistency for controlled/uncontrolled modes
The current interface design has an inconsistency where onPressedChange
is required but pressed
is optional. For better API design:
- In controlled mode, both
pressed
andonPressedChange
should be required - In uncontrolled mode, both should be optional
Consider using discriminated unions to enforce this:
interface BaseToggleProps {
children?: React.ReactNode;
className?: string;
label?: string;
disabled?: boolean;
}
interface ControlledToggleProps extends BaseToggleProps {
pressed: boolean;
onPressedChange: (isPressed: boolean) => void;
defaultPressed?: never;
}
interface UncontrolledToggleProps extends BaseToggleProps {
defaultPressed?: boolean;
pressed?: never;
onPressedChange?: never;
}
export type TogglePrimitiveProps = ControlledToggleProps | UncontrolledToggleProps;
⛔ Skipped due to learnings
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
29-39
: Add test coverage for disabled state handling
The event handler correctly implements the disabled state check, but this code path lacks test coverage.
Let's verify the current test coverage:
Would you like me to help generate test cases for:
- Clicking a disabled toggle doesn't trigger state changes
- Clicking a disabled toggle doesn't call onPressedChange
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-31: src/core/primitives/Toggle/index.tsx#L31
Added line #L31 was not covered by tests
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/core/primitives/Toggle/index.tsx (1)
50-52
: Improve typing for ARIA attributesInstead of using
any
, consider using a proper type for ARIA attributes.const ariaAttributes: React.AriaAttributes = { ...(label ? { 'aria-label': label } : {}), 'aria-pressed': isPressed ? 'true' : 'false', 'aria-disabled': disabled ? 'true' : 'false' };src/core/primitives/Toggle/tests/TogglePrimitive.test.js (2)
11-15
: Improve asChild test to follow RTL best practicesInstead of using
container.querySelector
, prefer using RTL queries that better reflect how users interact with your component.it('renders asChild correctly', () => { - const { container } = render(<TogglePrimitive asChild><button>Click me</button></TogglePrimitive>); - expect(container.querySelector('button')).toBeInTheDocument(); - expect(container.querySelector('button')).toHaveTextContent('Click me'); + render(<TogglePrimitive asChild><button>Click me</button></TogglePrimitive>); + const button = screen.getByRole('button'); + expect(button).toBeInTheDocument(); + expect(button).toHaveTextContent('Click me'); });
66-87
: Add test for controlled to uncontrolled mode transitionConsider adding a test case for when the
pressed
prop transitions from a boolean value to undefined, which should switch the component from controlled to uncontrolled mode.it('transitions from controlled to uncontrolled mode', () => { const { rerender } = render( <TogglePrimitive pressed={false}>Test Content</TogglePrimitive> ); const button = screen.getByRole('button'); expect(button).toHaveAttribute('data-state', 'off'); // Switch to uncontrolled mode rerender(<TogglePrimitive>Test Content</TogglePrimitive>); // Component should now respond to clicks fireEvent.click(button); expect(button).toHaveAttribute('data-state', 'on'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/core/batches.ts
(1 hunks)src/core/primitives/Toggle/index.tsx
(1 hunks)src/core/primitives/Toggle/tests/TogglePrimitive.test.js
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/batches.ts
🧰 Additional context used
📓 Learnings (1)
src/core/primitives/Toggle/index.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
🪛 GitHub Check: codecov/patch
src/core/primitives/Toggle/index.tsx
[warning] 31-31: src/core/primitives/Toggle/index.tsx#L31
Added line #L31 was not covered by tests
[warning] 45-46: src/core/primitives/Toggle/index.tsx#L45-L46
Added lines #L45 - L46 were not covered by tests
🔇 Additional comments (6)
src/core/primitives/Toggle/index.tsx (6)
15-23
: LGTM! Clean props destructuring with meaningful defaults
The props destructuring is well-organized with appropriate default values, and the renaming of pressed
to controlledPressed
improves code clarity.
24-28
: LGTM! Well-implemented state management pattern
The state management effectively handles both controlled and uncontrolled modes, with clear separation of concerns. The implementation aligns with the accepted pattern of not syncing internal state with external props in controlled mode.
54-62
: LGTM! Clean and accessible button implementation
The component rendering follows best practices with proper:
- Base button component usage
- Props spreading order
- Data attributes for styling
- ARIA attributes for accessibility
5-13
: 🛠️ Refactor suggestion
Refine prop types for consistent controlled/uncontrolled patterns
The current interface makes onPressedChange
required while pressed
is optional, which creates an inconsistent API. For proper controlled/uncontrolled patterns:
- In controlled mode, both
pressed
andonPressedChange
should be required - In uncontrolled mode, both should be optional
Consider using discriminated union types to enforce this:
type ControlledProps = {
pressed: boolean;
onPressedChange: (isPressed: boolean) => void;
defaultPressed?: never;
};
type UncontrolledProps = {
defaultPressed?: boolean;
pressed?: never;
onPressedChange?: never;
};
export interface TogglePrimitiveBaseProps {
children?: React.ReactNode;
className?: string;
label?: string;
disabled?: boolean;
}
export type TogglePrimitiveProps = TogglePrimitiveBaseProps & (ControlledProps | UncontrolledProps);
⛔ Skipped due to learnings
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
29-39
: Add test coverage for disabled state handling
The handlePressed
function's disabled state handling (line 31) is not covered by tests.
Let's verify the current test coverage:
Would you like me to help generate test cases for the disabled state handling?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-31: src/core/primitives/Toggle/index.tsx#L31
Added line #L31 was not covered by tests
41-48
: Address TODO comments about keyboard event handling
The TODO comments raise valid questions about keyboard event handling responsibility. Consider:
- Browser's native button element already handles space/enter keys
- ButtonPrimitive might be a better place for shared keyboard handling logic
Let's check if ButtonPrimitive already handles these events:
Consider moving keyboard event handling to ButtonPrimitive if it's a common requirement across button-based components.
✅ Verification successful
Keep keyboard event handling in Toggle component
Based on the codebase analysis:
- ButtonPrimitive implementation doesn't handle keyboard events
- Other interactive components (Tree, Tabs, Accordion) manage their own keyboard interactions
- The current implementation in Toggle follows the same pattern as other components in the codebase
The TODO can be resolved by keeping the keyboard event handling where it is, as this aligns with the existing architecture where each component manages its own keyboard interactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for keyboard event handling in ButtonPrimitive
ast-grep --pattern 'onKeyDown' -p 'src/**/ButtonPrimitive*'
Length of output: 226
Script:
#!/bin/bash
# Let's first locate ButtonPrimitive files
fd ButtonPrimitive
# Then examine their content for keyboard event handling
fd ButtonPrimitive --exec cat {}
# Also search for any keyboard event handling patterns in ButtonPrimitive
rg -A 3 "onKeyDown|handleKeyDown|keydown|KeyboardEvent" --type tsx
Length of output: 1398
Script:
#!/bin/bash
# Search for ButtonPrimitive implementation
fd "ButtonPrimitive.(tsx|ts|js|jsx)" --type f
# Look for keyboard event handling in all TypeScript/React files
rg -A 3 "onKeyDown|handleKeyDown|keydown|KeyboardEvent" --type ts --type jsx --type tsx
# Try to find the actual ButtonPrimitive component implementation
fd ButtonPrimitive --exec cat {} --type f
Length of output: 284
Script:
#!/bin/bash
# Find and examine ButtonPrimitive implementation
find . -name "ButtonPrimitive*" -type f -exec cat {} \;
# Search for keyboard event handling in TypeScript files
find . -type f -name "*.tsx" -o -name "*.ts" | xargs rg -A 3 "onKeyDown|handleKeyDown|keydown|KeyboardEvent"
Length of output: 13886
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-46: src/core/primitives/Toggle/index.tsx#L45-L46
Added lines #L45 - L46 were not covered by tests
This PR refactors the Toggle family components to use primitive
Summary by CodeRabbit
Release Notes
New Features
AlertDialog
,AvatarGroup
, andRadioGroup
available for import.TogglePrimitive
component with improved state management and accessibility features.ToggleGroup
for better interactivity.Bug Fixes
Toggle
component's props.AccordionItem
.Tests
TogglePrimitive
component to ensure proper rendering and functionality.Chores