Switch test #786
Switch test #786
Conversation
WalkthroughThis pull request introduces updates to the Switch component, focusing on documentation improvements, syntax refinements, and the addition of a new test suite. Changes include adjustments to import statements, restructuring of the Changes
Possibly related issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 6
🧹 Nitpick comments (4)
docs/app/docs/components/switch/page.js (1)
20-20: Remove commented-out code.Remove the commented-out UnderConstruction component as it's no longer needed.
- {/* <Documentation.UnderConstruction/> */}src/components/ui/Switch/tests/Switch.test.tsx (1)
5-30: Add more test coverage for Switch component.The current test suite covers basic functionality well, but consider adding these test cases:
- Test different variants ('classic', 'surface', 'solid')
- Test accessibility attributes (role, aria-label)
- Test keyboard interaction (Space/Enter keys)
- Test disabled state
Here's an example of additional test cases:
test('renders with different variants', () => { const variants = ['classic', 'surface', 'solid']; variants.forEach(variant => { const { rerender } = render(<Switch checked={true} onChange={() => {}} variant={variant} />); const switchElement = screen.getByRole('switch'); expect(switchElement).toHaveClass(`switch-${variant}`); rerender(<></>); }); }); test('handles keyboard interaction', () => { const handleChange = jest.fn(); render(<Switch checked={true} onChange={handleChange} />); const switchElement = screen.getByRole('switch'); fireEvent.keyDown(switchElement, { key: 'Enter' }); expect(handleChange).toHaveBeenCalledWith(false); fireEvent.keyDown(switchElement, { key: ' ' }); expect(handleChange).toHaveBeenCalledWith(true); }); test('respects disabled state', () => { const handleChange = jest.fn(); render(<Switch checked={true} onChange={handleChange} disabled />); const switchElement = screen.getByRole('switch'); expect(switchElement).toBeDisabled(); fireEvent.click(switchElement); expect(handleChange).not.toHaveBeenCalled(); });docs/app/docs/components/switch/docs/codeUsage.js (2)
4-19: Enhance example code with error handling and TypeScript.The example could be improved to demonstrate best practices:
- Add TypeScript for better type safety
- Include error handling
- Show more usage scenarios
-import Switch from "@radui/ui/Switch"; +import { Switch } from "@radui/ui/Switch"; +import { useState } from 'react'; -const SwitchExample = () => { - const [checked, setChecked] = React.useState(true) +const SwitchExample: React.FC = () => { + const [checked, setChecked] = useState<boolean>(true); + const [error, setError] = useState<string | null>(null); - const handleChange = (state) => { - setChecked(state) + const handleChange = (state: boolean) => { + try { + setChecked(state); + setError(null); + } catch (err) { + setError('Failed to update switch state'); + } } return ( <div> - <Switch checked={checked} onChange={handleChange} /> + <Switch + checked={checked} + onChange={handleChange} + aria-label="Toggle switch example" + /> + {error && <p role="alert" className="error">{error}</p>} </div>
Line range hint
23-67: Optimize SCSS styles for better maintainability.Consider these improvements to the styles:
- Use more specific transition properties
- Add hover and focus states for better accessibility
- Consider reducing selector specificity
.rad-ui-Switch { opacity: 0; position: absolute; + button{ position: relative; display: flex; align-items: center; font-size: 1.2rem; color: var(--rad-ui-color-accent-500); &::before{ content: ""; width: 2rem; height: 1.05rem; background-color: var(--rad-ui-color-accent-500); border-radius: 1rem; margin-right: .25rem; - transition: 200ms ease-in-out; + transition: background-color 200ms ease-in-out; } &::after{ position: absolute; left: .1rem; content: ""; width: .9rem; height: .9rem; background-color: var(--rad-ui-color-accent-50); border-radius: 1rem; - transition: 200ms ease-in-out; + transition: transform 200ms ease-in-out, background-color 200ms ease-in-out; } + &:hover::before { + background-color: var(--rad-ui-color-accent-600); + } + &:focus-visible { + outline: 2px solid var(--rad-ui-color-accent-500); + outline-offset: 2px; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/app/docs/components/switch/SwitchWrapper.js(1 hunks)docs/app/docs/components/switch/docs/codeUsage.js(1 hunks)docs/app/docs/components/switch/page.js(1 hunks)src/components/ui/Switch/Switch.tsx(1 hunks)src/components/ui/Switch/stories/Switch.stories.js(2 hunks)src/components/ui/Switch/tests/Switch.test.tsx(1 hunks)
🔇 Additional comments (2)
docs/app/docs/components/switch/SwitchWrapper.js (1)
8-12: LGTM! Clean state management implementation.The state management implementation follows React best practices with a clear state variable and handler function.
docs/app/docs/components/switch/page.js (1)
18-18: LGTM! Clean integration of SwitchWrapper.The SwitchWrapper component is properly integrated into the documentation.
| setIsChecked(state) | ||
| } | ||
|
|
||
| return <Switch checked={isChecked} onChange={handleChange} readOnly/> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant readOnly prop.
The readOnly prop contradicts the interactive nature of the component since you're already handling state changes via onChange.
- return <Switch checked={isChecked} onChange={handleChange} readOnly/>
+ return <Switch checked={isChecked} 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.
| return <Switch checked={isChecked} onChange={handleChange} readOnly/> | |
| return <Switch checked={isChecked} onChange={handleChange}/> |
| {variants.map((variant, index) => ( | ||
| <Switch defaultChecked={args} key={index} variant={variant} onChange={handleChange} {...args} /> | ||
| <Switch defaultChecked={isChecked} key={index} variant={variant} onChange={handleChange} {...args} /> |
There was a problem hiding this comment.
Fix controlled/uncontrolled component pattern mixing.
The Switch is currently mixing controlled and uncontrolled patterns by using both defaultChecked and onChange. This can lead to unexpected behavior. Use either controlled (checked) or uncontrolled (defaultChecked) pattern, but not both.
- <Switch defaultChecked={isChecked} key={index} variant={variant} onChange={handleChange} {...args} />
+ <Switch checked={isChecked} key={index} variant={variant} onChange={handleChange} {...args} />📝 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.
| <Switch defaultChecked={isChecked} key={index} variant={variant} onChange={handleChange} {...args} /> | |
| <Switch checked={isChecked} key={index} variant={variant} onChange={handleChange} {...args} /> |
src/components/ui/Switch/Switch.tsx
Outdated
| <div> | ||
| <input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked} | ||
| onChange={() => {}} | ||
| data-state={isChecked ? 'on' : 'off'} | ||
| aria-checked={isChecked} | ||
| /> | ||
| <button type="button" onClick={handleChecked} role="switch"></button> | ||
| </> | ||
| </div> |
There was a problem hiding this comment.
Fix accessibility issues and improve component structure.
- Having both an
inputand abuttonwith switch functionality violates accessibility guidelines. - The wrapper
divshould have proper styling context and ARIA attributes.
- <div>
+ <div className={className} role="presentation">
<input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked}
onChange={handleChange}
data-state={isChecked ? 'on' : 'off'}
aria-checked={isChecked}
/>
- <button type="button" onClick={handleChecked} role="switch"></button>
</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> | |
| <input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked} | |
| onChange={() => {}} | |
| data-state={isChecked ? 'on' : 'off'} | |
| aria-checked={isChecked} | |
| /> | |
| <button type="button" onClick={handleChecked} role="switch"></button> | |
| </> | |
| </div> | |
| <div className={className} role="presentation"> | |
| <input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked} | |
| onChange={handleChange} | |
| data-state={isChecked ? 'on' : 'off'} | |
| aria-checked={isChecked} | |
| /> | |
| </div> |
src/components/ui/Switch/Switch.tsx
Outdated
| const Switch = ({ children, customRootClass = '', className = '', defaultChecked=true, checked=true, onChange, ...props }:SwitchProps) => { | ||
|
|
||
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||
|
|
||
| const [isChecked, setIsChecked] = useState(checked || defaultChecked); | ||
|
|
||
| const [isChecked, setIsChecked] = useState(checked); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove default values for boolean props and simplify state management.
- Default values for
checkedanddefaultCheckedshould be removed as they could mask issues. - The
defaultCheckedprop is unused in state initialization.
-const Switch = ({ children, customRootClass = '', className = '', defaultChecked=true, checked=true, onChange, ...props }:SwitchProps) => {
+const Switch = ({ children, customRootClass = '', className = '', checked, onChange, ...props }:SwitchProps) => {📝 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 Switch = ({ children, customRootClass = '', className = '', defaultChecked=true, checked=true, onChange, ...props }:SwitchProps) => { | |
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | |
| const [isChecked, setIsChecked] = useState(checked || defaultChecked); | |
| const [isChecked, setIsChecked] = useState(checked); | |
| const Switch = ({ children, customRootClass = '', className = '', checked, onChange, ...props }:SwitchProps) => { | |
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | |
| const [isChecked, setIsChecked] = useState(checked); |
src/components/ui/Switch/Switch.tsx
Outdated
| const handleChecked = () => { | ||
| const updatedState = !isChecked; | ||
| setIsChecked(updatedState); | ||
| onChange(updatedState); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <input type='checkbox' className={clsx(rootClass)} {...props} checked= {isChecked}/> | ||
| <div> | ||
| <input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked} | ||
| onChange={() => {}} | ||
| data-state={isChecked ? 'on' : 'off'} | ||
| aria-checked={isChecked} | ||
| /> |
There was a problem hiding this comment.
Fix controlled component implementation and event handling.
- The empty onChange handler violates React's controlled component pattern.
- Having both
buttonandinputelements for the same action is redundant.
- const handleChecked = () => {
- const updatedState = !isChecked;
- setIsChecked(updatedState);
- onChange(updatedState);
- };
+ const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+ setIsChecked(event.target.checked);
+ onChange(event.target.checked);
+ };
return (
<div>
<input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked}
- onChange={() => {}}
+ onChange={handleChange}
data-state={isChecked ? 'on' : 'off'}
aria-checked={isChecked}
/>📝 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 handleChecked = () => { | |
| const updatedState = !isChecked; | |
| setIsChecked(updatedState); | |
| onChange(updatedState); | |
| }; | |
| return ( | |
| <> | |
| <input type='checkbox' className={clsx(rootClass)} {...props} checked= {isChecked}/> | |
| <div> | |
| <input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked} | |
| onChange={() => {}} | |
| data-state={isChecked ? 'on' : 'off'} | |
| aria-checked={isChecked} | |
| /> | |
| const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => { | |
| setIsChecked(event.target.checked); | |
| onChange(event.target.checked); | |
| }; | |
| return ( | |
| <div> | |
| <input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked} | |
| onChange={handleChange} | |
| data-state={isChecked ? 'on' : 'off'} | |
| aria-checked={isChecked} | |
| /> |
src/components/ui/Switch/Switch.tsx
Outdated
| defaultChecked?: boolean; | ||
| checked: boolean; | ||
| color: string; | ||
| children?: React.ReactNode; | ||
| className?: string; | ||
| customRootClass?: string; | ||
| onChange : (isChecked:boolean) => void; | ||
| props?: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing defaultChecked prop to avoid controlled/uncontrolled component confusion.
The component accepts both defaultChecked and checked props, which can lead to confusion about whether it's a controlled or uncontrolled component. Since checked is required and the component uses internal state, defaultChecked appears redundant.
export type SwitchProps = {
- defaultChecked?: boolean;
checked: boolean;
children?: React.ReactNode;
className?: string;
customRootClass?: string;
onChange : (isChecked:boolean) => void;
props?: any;
}📝 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.
| defaultChecked?: boolean; | |
| checked: boolean; | |
| color: string; | |
| children?: React.ReactNode; | |
| className?: string; | |
| customRootClass?: string; | |
| onChange : (isChecked:boolean) => void; | |
| props?: any; | |
| checked: boolean; | |
| children?: React.ReactNode; | |
| className?: string; | |
| customRootClass?: string; | |
| onChange : (isChecked:boolean) => void; | |
| props?: any; |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ui/Switch/Switch.tsx (1)
28-35:⚠️ Potential issueFix accessibility issues and component structure.
The current implementation has several issues:
- Having both an
inputand abuttonwith switch functionality violates accessibility guidelines- The wrapper
divlacks proper styling context and ARIA attributes- The empty onChange handler violates React patterns
Apply these changes:
- <div> + <div className={className} role="presentation"> <input type='checkbox' className={clsx(rootClass)} {...props} - checked={isChecked} - onChange={() => {}} + checked={checked} + onChange={handleChange} data-state={checked ? 'on' : 'off'} aria-checked={checked} /> - <button type="button" onClick={handleChecked} role="switch"></button> </div>
🧹 Nitpick comments (1)
src/components/ui/Switch/Switch.tsx (1)
16-16: Improve type safety by removing generic props.The component accepts arbitrary props through
props?: anywhich reduces type safety. Consider explicitly defining all expected HTML input attributes.-export type SwitchProps = { +export type SwitchProps = { + id?: string; + name?: string; + disabled?: boolean; checked: boolean; children?: React.ReactNode; className?: string; customRootClass?: string; onChange: (isChecked: boolean) => void; - props?: any; -} +} & Omit<React.InputHTMLAttributes<HTMLInputElement>, 'onChange' | 'checked'>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/app/docs/components/switch/SwitchWrapper.js(1 hunks)docs/app/docs/components/switch/page.js(1 hunks)src/components/ui/Switch/Switch.tsx(1 hunks)src/components/ui/Switch/stories/Switch.stories.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/app/docs/components/switch/page.js
- docs/app/docs/components/switch/SwitchWrapper.js
- src/components/ui/Switch/stories/Switch.stories.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/components/ui/Switch/Switch.tsx (1)
19-24:⚠️ Potential issueFix controlled component implementation.
The component mixes controlled and uncontrolled patterns which can lead to state synchronization issues. The component should either:
- Be fully controlled by parent (recommended)
- Or be fully uncontrolled using defaultChecked
For a controlled component implementation:
-const [isChecked, setIsChecked] = useState(checked); - -const handleChecked = () => { - const updatedState = !isChecked; - setIsChecked(updatedState); - onChange(updatedState) -}; +const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => { + onChange(event.target.checked); +};Likely invalid or redundant comment.
src/components/ui/Switch/Switch.tsx
Outdated
| children?: React.ReactNode; | ||
| className?: string; | ||
| customRootClass?: string; | ||
| onChange : (isChecked:boolean) => void; | ||
| props?: any; | ||
| } | ||
|
|
||
| const Switch = ({ children, customRootClass = '', className = '', color = '', defaultChecked, checked, onChange, ...props }:SwitchProps) => { | ||
| const Switch = ({ children, customRootClass = '', className = '', checked, onChange, ...props }:SwitchProps) => { |
There was a problem hiding this comment.
why did you remove the defaultChecked?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/ui/Switch/Switch.tsx (2)
8-14: Improve type safety and documentation for props.The component accepts both controlled (
checked) and uncontrolled (defaultChecked) modes, but the types could be more explicit:export type SwitchProps = { - defaultChecked?: boolean; - checked: boolean; + defaultChecked?: boolean; + checked?: boolean; children?: React.ReactNode; className?: string; customRootClass?: string; - onChange : (isChecked:boolean) => void; + onChange?: (isChecked: boolean) => void; props?: any; }Also consider:
- Making
onChangeoptional for uncontrolled mode- Adding JSDoc comments to document the controlled/uncontrolled usage
- Removing the generic
props?: anyin favor of specific HTML input attributes
17-19: Fix code formatting.Remove unnecessary blank lines to comply with ESLint rules:
const Switch = ({ children, customRootClass = '', className = '', defaultChecked, checked, onChange, ...props }:SwitchProps) => { - const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME);🧰 Tools
🪛 eslint
[error] 17-19: Block must not be padded by blank lines.
(padded-blocks)
src/components/ui/Switch/tests/Switch.test.tsx (1)
5-11: Enhance test coverage and organization.While the basic test cases are good, consider adding:
- Error cases (e.g., invalid props)
- Keyboard interaction tests
- Custom class and styling tests
- Snapshot tests
Also, organize tests better using nested describes:
describe('Switch Component', () => { - test('renders correctly', () => { + describe('rendering', () => { + test('renders correctly', () => { + render(<Switch checked={true} onChange={() => {}}/>) + const inputElement = screen.getByRole('checkbox') + expect(inputElement).toBeInTheDocument(); + expect(inputElement).toHaveAttribute('role', 'switch'); + }) + })🧰 Tools
🪛 eslint
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Switch/Switch.tsx(1 hunks)src/components/ui/Switch/stories/Switch.stories.js(2 hunks)src/components/ui/Switch/tests/Switch.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Switch/stories/Switch.stories.js
🧰 Additional context used
🪛 eslint
src/components/ui/Switch/tests/Switch.test.tsx
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
[error] 47-49: Block must not be padded by blank lines.
(padded-blocks)
[error] 49-49: Newline required at end of file but not found.
(eol-last)
src/components/ui/Switch/Switch.tsx
[error] 17-19: Block must not be padded by blank lines.
(padded-blocks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/components/ui/Switch/Switch.tsx (3)
22-26:⚠️ Potential issueFix event handling and controlled component behavior.
The current implementation has several issues:
- Empty onChange handler on input
- Duplicate click handlers
- Doesn't properly handle controlled mode
- const handleChecked = () => { - const updatedState = !isChecked; - setIsChecked(updatedState); - onChange(updatedState) - }; + const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => { + if (checked === undefined) { + // Uncontrolled mode + setIsChecked(event.target.checked); + } + onChange?.(event.target.checked); + };Likely invalid or redundant comment.
17-21:⚠️ Potential issueFix controlled/uncontrolled component implementation.
The current state management doesn't properly handle controlled vs uncontrolled modes:
-const Switch = ({ children, customRootClass = '', className = '', defaultChecked, checked, onChange, ...props }:SwitchProps) => { +const Switch = ({ children, customRootClass = '', className = '', defaultChecked = false, checked, onChange, ...props }:SwitchProps) => { const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); - const [isChecked, setIsChecked] = useState(checked || defaultChecked); + const [isChecked, setIsChecked] = useState(defaultChecked); + + // In controlled mode, sync with external checked prop + React.useEffect(() => { + if (checked !== undefined) { + setIsChecked(checked); + } + }, [checked]);Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 17-19: Block must not be padded by blank lines.
(padded-blocks)
29-36:⚠️ Potential issueFix accessibility implementation and remove redundant elements.
The current implementation violates accessibility guidelines by having both an input and button for the same action:
- <div> + <div + className={className} + role="presentation" + > <input type='checkbox' className={clsx(rootClass)} {...props} checked={isChecked} - onChange={() => {}} + onChange={handleChange} data-state={isChecked ? 'on' : 'off'} aria-checked={isChecked} + role="switch" + aria-label={props['aria-label'] || 'Toggle switch'} /> - <button type="button" onClick={handleChecked} role="switch"></button> </div>Likely invalid or redundant comment.
| test('toggles state on click', () => { | ||
| const handleChange = jest.fn(); | ||
| render(<Switch checked={true} onChange={handleChange}/>) | ||
| const switchElement = screen.getByRole('switch') | ||
|
|
||
| fireEvent.click(switchElement) | ||
| expect(handleChange).toHaveBeenCalledWith(false) | ||
|
|
||
| fireEvent.click(switchElement) | ||
| expect(handleChange).toHaveBeenCalledWith(true) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve controlled mode test coverage.
The toggle state test needs improvement:
- Inconsistent role usage (switch vs checkbox)
- Missing important controlled mode scenarios
- test('toggles state on click', () => {
+ describe('controlled mode', () => {
+ test('calls onChange with correct values', () => {
const handleChange = jest.fn();
render(<Switch checked={true} onChange={handleChange}/>)
- const switchElement = screen.getByRole('switch')
+ const switchElement = screen.getByRole('checkbox')
fireEvent.click(switchElement)
expect(handleChange).toHaveBeenCalledWith(false)
fireEvent.click(switchElement)
expect(handleChange).toHaveBeenCalledWith(true)
- })
+ })
+
+ test('stays in sync with checked prop', () => {
+ const { rerender } = render(<Switch checked={true} onChange={() => {}}/>)
+ const switchElement = screen.getByRole('checkbox')
+ expect(switchElement).toBeChecked()
+
+ rerender(<Switch checked={false} onChange={() => {}}/>)
+ expect(switchElement).not.toBeChecked()
+ })
+ })📝 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.
| test('toggles state on click', () => { | |
| const handleChange = jest.fn(); | |
| render(<Switch checked={true} onChange={handleChange}/>) | |
| const switchElement = screen.getByRole('switch') | |
| fireEvent.click(switchElement) | |
| expect(handleChange).toHaveBeenCalledWith(false) | |
| fireEvent.click(switchElement) | |
| expect(handleChange).toHaveBeenCalledWith(true) | |
| }) | |
| describe('controlled mode', () => { | |
| test('calls onChange with correct values', () => { | |
| const handleChange = jest.fn(); | |
| render(<Switch checked={true} onChange={handleChange}/>) | |
| const switchElement = screen.getByRole('checkbox') | |
| fireEvent.click(switchElement) | |
| expect(handleChange).toHaveBeenCalledWith(false) | |
| fireEvent.click(switchElement) | |
| expect(handleChange).toHaveBeenCalledWith(true) | |
| }) | |
| test('stays in sync with checked prop', () => { | |
| const { rerender } = render(<Switch checked={true} onChange={() => {}}/>) | |
| const switchElement = screen.getByRole('checkbox') | |
| expect(switchElement).toBeChecked() | |
| rerender(<Switch checked={false} onChange={() => {}}/>) | |
| expect(switchElement).not.toBeChecked() | |
| }) | |
| }) |
| test('toggle state on when clicked', () => { | ||
| render(<Switch checked={true} onChange={() => {}} defaultChecked={true} />) | ||
| const checkbox = screen.getByRole('checkbox') | ||
|
|
||
| expect(checkbox).toBeChecked() | ||
|
|
||
| fireEvent.click(checkbox) | ||
| expect(checkbox).toBeChecked() | ||
| }) | ||
|
|
||
| test('toggle state off when clicked again', () => { | ||
| render(<Switch checked={false} onChange={() => {}} defaultChecked={false} />) | ||
| const checkbox = screen.getByRole('checkbox') | ||
|
|
||
| fireEvent.click(checkbox) | ||
| expect(checkbox).not.toBeChecked() | ||
| }) |
There was a problem hiding this comment.
Fix uncontrolled mode tests.
The current tests mix controlled and uncontrolled props, which is incorrect:
- test('toggle state on when clicked', () => {
- render(<Switch checked={true} onChange={() => {}} defaultChecked={true} />)
+ describe('uncontrolled mode', () => {
+ test('initializes with defaultChecked', () => {
+ render(<Switch defaultChecked={true} />)
+ const checkbox = screen.getByRole('checkbox')
+ expect(checkbox).toBeChecked()
+ })
+
+ test('toggles state independently', () => {
+ render(<Switch defaultChecked={false} />)
const checkbox = screen.getByRole('checkbox')
+ expect(checkbox).not.toBeChecked()
- expect(checkbox).toBeChecked()
-
- fireEvent.click(checkbox)
- expect(checkbox).toBeChecked()
- })
-
- test('toggle state off when clicked again', () => {
- render(<Switch checked={false} onChange={() => {}} defaultChecked={false} />)
- const checkbox = screen.getByRole('checkbox')
-
- fireEvent.click(checkbox)
- expect(checkbox).not.toBeChecked()
- })
+ fireEvent.click(checkbox)
+ expect(checkbox).toBeChecked()
+
+ fireEvent.click(checkbox)
+ expect(checkbox).not.toBeChecked()
+ })
+ })📝 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.
| test('toggle state on when clicked', () => { | |
| render(<Switch checked={true} onChange={() => {}} defaultChecked={true} />) | |
| const checkbox = screen.getByRole('checkbox') | |
| expect(checkbox).toBeChecked() | |
| fireEvent.click(checkbox) | |
| expect(checkbox).toBeChecked() | |
| }) | |
| test('toggle state off when clicked again', () => { | |
| render(<Switch checked={false} onChange={() => {}} defaultChecked={false} />) | |
| const checkbox = screen.getByRole('checkbox') | |
| fireEvent.click(checkbox) | |
| expect(checkbox).not.toBeChecked() | |
| }) | |
| describe('uncontrolled mode', () => { | |
| test('initializes with defaultChecked', () => { | |
| render(<Switch defaultChecked={true} />) | |
| const checkbox = screen.getByRole('checkbox') | |
| expect(checkbox).toBeChecked() | |
| }) | |
| test('toggles state independently', () => { | |
| render(<Switch defaultChecked={false} />) | |
| const checkbox = screen.getByRole('checkbox') | |
| expect(checkbox).not.toBeChecked() | |
| fireEvent.click(checkbox) | |
| expect(checkbox).toBeChecked() | |
| fireEvent.click(checkbox) | |
| expect(checkbox).not.toBeChecked() | |
| }) | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/ui/Switch/tests/Switch.test.tsx (2)
13-23:⚠️ Potential issueFix role inconsistency and improve controlled mode coverage.
The test uses 'switch' role while other tests use 'checkbox' role. Additionally, it's missing important controlled mode scenarios.
This issue was previously identified. Please refer to the earlier review comment about improving controlled mode test coverage, which suggests:
- Using consistent role ('checkbox')
- Adding test for checked prop sync
25-41:⚠️ Potential issueFix incorrect prop combination in uncontrolled mode tests.
The tests incorrectly mix controlled (
checked) and uncontrolled (defaultChecked) props, which can lead to unexpected behavior.This issue was previously identified. Please refer to the earlier review comment about fixing uncontrolled mode tests, which provides a complete solution for properly testing uncontrolled behavior.
🧹 Nitpick comments (1)
src/components/ui/Switch/tests/Switch.test.tsx (1)
5-43: Fix formatting issues.Please address the following formatting issues:
- Remove extra blank lines after the describe block start (line 5-7)
- Remove extra blank lines before the describe block end (line 41-43)
- Add newline at end of file
describe('Switch Component', () => { - test('renders correctly', () => { // ... tests ... }) - }) +🧰 Tools
🪛 eslint
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
[error] 41-43: Block must not be padded by blank lines.
(padded-blocks)
[error] 43-43: Newline required at end of file but not found.
(eol-last)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/components/ui/Switch/tests/Switch.test.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Switch/tests/Switch.test.tsx
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
[error] 41-43: Block must not be padded by blank lines.
(padded-blocks)
[error] 43-43: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (1)
src/components/ui/Switch/tests/Switch.test.tsx (1)
1-5: LGTM! Clean and standard test setup.The imports and test suite setup follow best practices.
| test('renders correctly', () => { | ||
| render(<Switch checked={true} onChange={() => {}} color='' />) | ||
| const inputElement = screen.getByRole('checkbox') | ||
| expect(inputElement).toBeInTheDocument(); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the basic render test with meaningful assertions.
The current test only verifies component presence. Consider adding:
- Accessibility attributes verification
- Default state checks
- Meaningful color prop value
test('renders correctly', () => {
- render(<Switch checked={true} onChange={() => {}} color='' />)
- const inputElement = screen.getByRole('checkbox')
- expect(inputElement).toBeInTheDocument();
+ render(<Switch checked={true} onChange={() => {}} color='primary' />)
+ const inputElement = screen.getByRole('checkbox')
+ expect(inputElement).toBeInTheDocument()
+ expect(inputElement).toHaveAttribute('aria-checked', 'true')
+ expect(inputElement).toHaveAttribute('type', 'checkbox')
+ expect(inputElement).toBeEnabled()
})📝 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.
| test('renders correctly', () => { | |
| render(<Switch checked={true} onChange={() => {}} color='' />) | |
| const inputElement = screen.getByRole('checkbox') | |
| expect(inputElement).toBeInTheDocument(); | |
| }) | |
| test('renders correctly', () => { | |
| render(<Switch checked={true} onChange={() => {}} color='primary' />) | |
| const inputElement = screen.getByRole('checkbox') | |
| expect(inputElement).toBeInTheDocument() | |
| expect(inputElement).toHaveAttribute('aria-checked', 'true') | |
| expect(inputElement).toHaveAttribute('type', 'checkbox') | |
| expect(inputElement).toBeEnabled() | |
| }) |
| }) | ||
|
|
||
| test('renders in controlled mode correctly', () => { | ||
| const handleChange = jest.fn(); |
There was a problem hiding this comment.
for controlled mode, create a state with useState and pass it to the component
when switch is toggled, check the state variable
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ui/Switch/stories/Switch.stories.js (1)
39-45: Simplify uncontrolled component implementation and fix formatting.The uncontrolled story can be simplified by removing the unnecessary onChange handler. Also, there are some formatting issues to address.
export const Uncontrolled = () => { - - return <SandboxEditor> - <Switch defaultChecked ={true} onChange={() => {}}/> - - </SandboxEditor> + return <SandboxEditor> + <Switch defaultChecked={true} /> + </SandboxEditor> } +🧰 Tools
🪛 eslint
[error] 39-41: Block must not be padded by blank lines.
(padded-blocks)
[error] 45-45: Newline required at end of file but not found.
(eol-last)
src/components/ui/Switch/tests/Switch.test.tsx (1)
25-42: Clean up uncontrolled mode tests.The tests can be simplified by removing unnecessary onChange handlers and fixing formatting.
-test('renders in uncontrolled mode correctly with defaultChecked', () => { +describe('uncontrolled mode', () => { + test('initializes with defaultChecked', () => { + render(<Switch defaultChecked={true} />) + const checkbox = screen.getByRole('checkbox') + expect(checkbox).toBeChecked() + }) - render(<Switch defaultChecked={true} onChange={() => {}}/>) - const checkbox = screen.getByRole('checkbox') - expect(checkbox).toBeChecked() - }) - - test('toggles state independently',() => { - render(<Switch defaultChecked={false} onChange={() => {}} />) - const checkbox = screen.getByRole('checkbox') - expect(checkbox).not.toBeChecked() + test('toggles state independently', () => { + render(<Switch defaultChecked={false} />) + const checkbox = screen.getByRole('checkbox') + expect(checkbox).not.toBeChecked() - fireEvent.click(checkbox) - expect(checkbox).toBeChecked() - - fireEvent.click(checkbox) - expect(checkbox).not.toBeChecked() - }) + fireEvent.click(checkbox) + expect(checkbox).toBeChecked() + + fireEvent.click(checkbox) + expect(checkbox).not.toBeChecked() + }) +})🧰 Tools
🪛 eslint
[error] 25-27: Block must not be padded by blank lines.
(padded-blocks)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Switch/Switch.tsx(2 hunks)src/components/ui/Switch/stories/Switch.stories.js(3 hunks)src/components/ui/Switch/tests/Switch.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Switch/Switch.tsx
🧰 Additional context used
🪛 eslint
src/components/ui/Switch/stories/Switch.stories.js
[error] 39-41: Block must not be padded by blank lines.
(padded-blocks)
[error] 45-45: Newline required at end of file but not found.
(eol-last)
src/components/ui/Switch/tests/Switch.test.tsx
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
[error] 25-27: Block must not be padded by blank lines.
(padded-blocks)
[error] 42-44: Block must not be padded by blank lines.
(padded-blocks)
[error] 44-44: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (4)
src/components/ui/Switch/stories/Switch.stories.js (2)
Line range hint
13-23: Fix controlled/uncontrolled component pattern mixing.The component is still mixing controlled and uncontrolled patterns by using both
defaultCheckedand state management. This can lead to unexpected behavior.- const [isChecked, setIsChecked] = useState(true) - - const handleChange = (state) => { - setIsChecked(state); - }; - return <SandboxEditor className="flex flex-col gap-2"> - {variants.map((variant, index) => ( - <Switch defaultChecked={args} key={index} variant={variant} onChange={handleChange} {...args} /> - ))} + return <SandboxEditor className="flex flex-col gap-2"> + {variants.map((variant, index) => ( + <Switch key={index} variant={variant} {...args} /> + ))}
28-37: LGTM! Well-implemented controlled component pattern.The controlled story demonstrates the correct usage pattern with proper state management and onChange handling.
src/components/ui/Switch/tests/Switch.test.tsx (2)
7-11: Enhance basic render test with accessibility checks.The test should verify accessibility attributes and meaningful props.
test('renders correctly', () => { - render(<Switch checked={true} onChange={() => {}} />) - const inputElement = screen.getByRole('checkbox') - expect(inputElement).toBeInTheDocument(); + render(<Switch checked={true} onChange={() => {}} color="primary" />) + const inputElement = screen.getByRole('checkbox') + expect(inputElement).toBeInTheDocument() + expect(inputElement).toHaveAttribute('aria-checked', 'true') + expect(inputElement).toHaveAttribute('type', 'checkbox') + expect(inputElement).toBeEnabled() })
13-23: Improve controlled mode test implementation.The test needs improvements in role consistency and state management verification.
-test('renders in controlled mode correctly', () => { +describe('controlled mode', () => { + test('calls onChange with correct values', () => { const handleChange = jest.fn(); render(<Switch checked={true} onChange={handleChange} />) - const switchElement = screen.getByRole('switch') + const switchElement = screen.getByRole('checkbox') fireEvent.click(switchElement) expect(handleChange).toHaveBeenCalledWith(false) fireEvent.click(switchElement) expect(handleChange).toHaveBeenCalledWith(true) - }) + }) + + test('stays in sync with state', () => { + const TestComponent = () => { + const [checked, setChecked] = React.useState(true) + return <Switch checked={checked} onChange={setChecked} /> + } + render(<TestComponent />) + const switchElement = screen.getByRole('checkbox') + + expect(switchElement).toBeChecked() + fireEvent.click(switchElement) + expect(switchElement).not.toBeChecked() + }) +})
Refactor and add test cases for switch component
Summary by CodeRabbit
Documentation
Tests
Chores