Feature toggle primitive #553
Conversation
WalkthroughThe changes introduce enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@jindaljyoti has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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/fragments/TogglePrimitiveRoot.tsxOops! 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
src/core/primitives/Toggle/contexts/TogglePrimitiveContext.tsx (1)
1-9: Consider these architectural patterns for toggle component integrationAs you proceed with integrating this context into the toggle component, consider the following architectural patterns:
- Keep the primitive focused on core toggle logic (state, accessibility) while the component handles presentation
- Consider implementing a compound component pattern:
<Toggle.Root> <Toggle.Thumb /> <Toggle.Label /> </Toggle.Root>- Ensure the primitive handles ARIA attributes (aria-pressed, role="switch")
- Consider adding event callbacks for state changes to support controlled and uncontrolled modes
Would you like me to provide a detailed implementation example for any of these patterns?
src/core/primitives/Toggle/fragments/TogglePrimitiveRoot.tsx (1)
1-36: Consider extracting toggle logic into a custom hookTo improve reusability and testing, consider extracting the toggle logic into a custom hook (e.g.,
useTogglePrimitive). This would:
- Separate concerns between state management and rendering
- Make the logic reusable across different components
- Make it easier to test the logic in isolation
Example implementation:
function useTogglePrimitive({ pressed, defaultPressed, onChange }: Pick<TogglePrimitiveRootProps, 'pressed' | 'defaultPressed' | 'onChange'>) { const [uncontrolledPressed, setUncontrolledPressed] = useState(defaultPressed ?? false); const isPressed = pressed ?? uncontrolledPressed; const handlePressed = useCallback((event: React.MouseEvent | React.KeyboardEvent) => { if (event.type === 'keydown' && (event as React.KeyboardEvent).key !== 'Enter' && (event as React.KeyboardEvent).key !== ' ') { return; } const updatedPressed = !isPressed; setUncontrolledPressed(updatedPressed); onChange(updatedPressed); }, [isPressed, onChange]); return { isPressed, handlePressed }; }src/components/ui/Toggle/Toggle.tsx (4)
Line range hint
8-17: Simplify thedefaultPressedtype definitionThe type union
boolean | falseis redundant sincebooleanalready includesfalse. Consider simplifying it to justboolean.- defaultPressed? : boolean | false ; + defaultPressed? : boolean;
Line range hint
33-33: Fix incorrect state initialization logicThe current initialization
pressed || defaultPressedhas a logical flaw. WhenpressedisfalseanddefaultPressedistrue, it will usedefaultPressed, which contradicts the expected behavior wherepressedshould always take precedence overdefaultPressed.- const [isPressed, setIsPressed] = useState(pressed || defaultPressed); + const [isPressed, setIsPressed] = useState(pressed ?? defaultPressed ?? false);
Line range hint
33-39: Implement proper controlled/uncontrolled component patternThe component currently mixes controlled and uncontrolled patterns, which can lead to state synchronization issues. The internal state might get out of sync with the external
pressedprop.Consider implementing either:
- Fully controlled component (recommended):
- const [isPressed, setIsPressed] = useState(pressed || defaultPressed); - const handlePressed = () => { - const updatedPressed = !isPressed; - setIsPressed(updatedPressed); - onChange(updatedPressed); + onChange(!pressed); };
- Or proper uncontrolled component with key prop:
+ const [isPressed, setIsPressed] = useState(defaultPressed ?? false); + + // Sync with external updates + useEffect(() => { + if (pressed !== undefined) { + setIsPressed(pressed); + } + }, [pressed]); + const handlePressed = () => { const updatedPressed = !isPressed; setIsPressed(updatedPressed); onChange(updatedPressed); };
Line range hint
1-55: Consider refactoring to use the new TogglePrimitiveBased on the PR objectives, this component should be refactored to utilize the newly created toggle primitive instead of maintaining its own state management logic. This would ensure consistency and reduce code duplication.
Consider:
- Importing and using the
TogglePrimitivecomponent- Delegating state management to the primitive
- Focusing this component on styling and specific UI requirements
Example approach:
import { TogglePrimitive } from '~/core/primitives/Toggle'; const Toggle: React.FC<ToggleProps> = (props) => { return ( <TogglePrimitive.Root defaultPressed={props.defaultPressed} pressed={props.pressed} onPressedChange={props.onChange} > <ButtonPrimitive className={props.className} {...props}> {props.children} </ButtonPrimitive> </TogglePrimitive.Root> ); };src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (6)
Line range hint
6-6: Define proper TypeScript interface for component propsUsing
:anytype annotation reduces type safety and makes the component harder to maintain. Consider defining a proper interface for the props.+interface ToggleItemProps { + children: React.ReactNode; + value?: string | number | null; + [key: string]: any; // for remaining props if needed +} -const ToggleItem = ({ children, value = null, ...props }:any) => { +const ToggleItem = ({ children, value = null, ...props }: ToggleItemProps) => {
11-11: Remove unnecessary empty line-
12-12: Add error handling for missing contextThe optional chaining is good, but consider adding a check for required context to fail fast and provide better error messages.
+if (!toggleContext) { + throw new Error('ToggleItem must be used within a ToggleContext provider'); +} + const isActive = toggleContext?.activeToggles?.includes(value);
20-20: Remove unnecessary empty line-
Line range hint
14-35: Simplify toggle selection logicThe current implementation can be made more concise and consistent by removing early returns and unifying the logic structure.
const handleToggleSelect = () => { - let activeToggleArray = toggleContext?.activeToggles || []; + if (!toggleContext) return; + + const { activeToggles = [], setActiveToggles } = toggleContext; - // For Single Case if (type === 'single') { - if (isActive) { - - toggleContext?.setActiveToggles([]); - return; - } else { - toggleContext?.setActiveToggles([value]); - return; - } + setActiveToggles(isActive ? [] : [value]); + return; } - // For Multiple Case - if (type === 'multiple') { - if (isActive) { - activeToggleArray = activeToggleArray.filter((item: any) => item !== value); - } else { - activeToggleArray = [...activeToggleArray, value]; // Using spread operator to create a new array - } - } - - toggleContext?.setActiveToggles(activeToggleArray); + const newToggles = isActive + ? activeToggles.filter((item: any) => item !== value) + : [...activeToggles, value]; + + setActiveToggles(newToggles); };
Line range hint
37-52: Optimize component rendering and stylingConsider these improvements for better performance and maintainability:
- Memoize the event handler
- Use CSS classes instead of inline styles
- Simplify aria-pressed assignment
+import { useCallback } from 'react'; +import clsx from 'clsx'; // Add this to your dependencies if not present const ToggleItem = ({ children, value = null, ...props }:any) => { // ... existing code ... + const handleClick = useCallback(() => { + handleToggleSelect(); + }, [handleToggleSelect]); + - if (isActive) { - props['aria-pressed'] = 'true'; - } else { - props['aria-pressed'] = 'false'; - } + props['aria-pressed'] = String(!!isActive); return <ButtonPrimitive - className={`${isActive ? 'bg-blue-600' : ''}`} - onClick={() => { - handleToggleSelect(); - }} + className={clsx('toggle-item', isActive && 'toggle-item--active')} + onClick={handleClick} {...props} >{children}</ButtonPrimitive>; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/components/ui/Toggle/Toggle.tsx(3 hunks)src/components/ui/ToggleGroup/fragments/ToggleItem.tsx(2 hunks)src/core/primitives/Toggle/contexts/TogglePrimitiveContext.tsx(1 hunks)src/core/primitives/Toggle/fragments/TogglePrimitiveRoot.tsx(1 hunks)src/core/primitives/Toggle/index.tsx(1 hunks)src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/core/primitives/Toggle/index.tsx
- src/core/primitives/Toggle/stories/TogglePrimitive.stories.tsx
🔇 Additional comments (3)
src/core/primitives/Toggle/contexts/TogglePrimitiveContext.tsx (1)
1-1: LGTM!
The import statement is clean and follows React conventions.
src/core/primitives/Toggle/fragments/TogglePrimitiveRoot.tsx (1)
1-4: Verify path alias configuration
The import statement uses a path alias (~). Ensure that this alias is properly configured in your build setup (tsconfig.json/webpack config).
✅ Verification successful
Path alias ~ is correctly configured
The path alias ~ is properly configured in the tsconfig.json file, mapping to the src/* directory. The import statement ~/core/customClassSwitcher will correctly resolve to src/core/customClassSwitcher.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for path alias configuration in tsconfig and webpack files
# Check tsconfig.json for path alias
fd tsconfig.json --exec cat {} | jq -r '.compilerOptions.paths | select(. != null)'
# Check webpack config for path alias
fd 'webpack.config.*' --exec cat {} | grep -A 5 'alias'
Length of output: 205
src/components/ui/ToggleGroup/fragments/ToggleItem.tsx (1)
Line range hint 1-52: Verify integration with TogglePrimitive
The PR objectives mention implementing a toggle primitive, but this component still uses the old toggle context. Consider migrating to use the new toggle primitive for consistency.
| interface TogglePrimitiveContextType { | ||
| rootClass: string; | ||
| isPressed: boolean | undefined; | ||
| handlePressed: () => void; | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and improve type safety
The interface could benefit from the following improvements:
- Add JSDoc documentation
- Consider renaming
rootClassto be more specific (e.g.,toggleRootClass) - Make properties readonly to prevent accidental mutations
- Consider adding parameters to
handlePressedfor more flexibility
Here's the suggested implementation:
+/**
+ * Context type for the Toggle Primitive
+ * @property toggleRootClass - CSS class for the root element
+ * @property isPressed - Current pressed state
+ * @property handlePressed - Callback to handle toggle state changes
+ */
interface TogglePrimitiveContextType {
- rootClass: string;
- isPressed: boolean | undefined;
- handlePressed: () => void;
+ readonly toggleRootClass: string;
+ readonly isPressed: boolean | undefined;
+ readonly handlePressed: (value?: boolean) => 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.
| interface TogglePrimitiveContextType { | |
| rootClass: string; | |
| isPressed: boolean | undefined; | |
| handlePressed: () => void; | |
| } | |
| /** | |
| * Context type for the Toggle Primitive | |
| * @property toggleRootClass - CSS class for the root element | |
| * @property isPressed - Current pressed state | |
| * @property handlePressed - Callback to handle toggle state changes | |
| */ | |
| interface TogglePrimitiveContextType { | |
| readonly toggleRootClass: string; | |
| readonly isPressed: boolean | undefined; | |
| readonly handlePressed: (value?: boolean) => void; | |
| } |
| className={`${rootClass}`} onClick ={handlePressed} | ||
| data-state={isPressed ? 'on' : 'off'} | ||
| type="button" | ||
| type='button' | ||
| data-disabled={props.disabled ? '' : undefined} | ||
| aria-pressed={pressed} {...props}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix accessibility and simplify attribute handling
- The
aria-pressedattribute should reflect the actual state of the toggle, not just thepressedprop. - The
data-disabledimplementation can be simplified.
<ButtonPrimitive
className={`${rootClass}`} onClick={handlePressed}
data-state={isPressed ? 'on' : 'off'}
type='button'
- data-disabled={props.disabled ? '' : undefined}
- aria-pressed={pressed}
+ data-disabled={props.disabled || undefined}
+ aria-pressed={isPressed}
{...props}>📝 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.
| className={`${rootClass}`} onClick ={handlePressed} | |
| data-state={isPressed ? 'on' : 'off'} | |
| type="button" | |
| type='button' | |
| data-disabled={props.disabled ? '' : undefined} | |
| aria-pressed={pressed} {...props}> | |
| className={`${rootClass}`} onClick ={handlePressed} | |
| data-state={isPressed ? 'on' : 'off'} | |
| type='button' | |
| data-disabled={props.disabled || undefined} | |
| aria-pressed={isPressed} {...props}> |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/core/primitives/Toggle/fragments/TogglePrimitiveRoot.tsx (1)
1-30: Consider component composition patternSince this is a primitive component that will be used by the UI toggle component:
- Consider splitting into smaller subcomponents (Root, Thumb, Track) for better composition
- Implement a compound component pattern to allow flexible styling and behavior customization
Example structure:
export const TogglePrimitive = { Root: TogglePrimitiveRoot, Thumb: TogglePrimitiveThumb, Track: TogglePrimitiveTrack, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/core/primitives/Toggle/contexts/TogglePrimitiveContext.tsx(1 hunks)src/core/primitives/Toggle/fragments/TogglePrimitiveRoot.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/primitives/Toggle/contexts/TogglePrimitiveContext.tsx
| const values = { | ||
| handlePressed, | ||
| isPressed, | ||
| setIsPressed | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary context setup
As per reviewer feedback, context is not needed for this primitive component:
- const values = {
- handlePressed,
- isPressed,
- setIsPressed
- };📝 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 values = { | |
| handlePressed, | |
| isPressed, | |
| setIsPressed | |
| }; |
| setIsPressed | ||
| }; | ||
|
|
||
| return <span className={className}{...props}>{children}</span> |
There was a problem hiding this comment.
Add proper accessibility attributes
The toggle element needs proper ARIA attributes and event handlers:
- return <span className={className}{...props}>{children}</span>
+ return (
+ <span
+ role="button"
+ tabIndex={0}
+ aria-pressed={isPressed}
+ className={className}
+ onClick={handlePressed}
+ onKeyDown={handlePressed}
+ >
+ {children}
+ </span>
+ );Committable suggestion skipped: line range outside the PR's diff.
| export interface TogglePrimitiveRootProps { | ||
| defaultPressed? : boolean | false; | ||
| pressed: boolean; | ||
| children?: React.ReactNode; | ||
| className?: string; | ||
| onChange : (isPressed:boolean) => void; | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix prop types and add documentation
The interface needs improvements for better type safety and documentation:
+/**
+ * Props for the Toggle Primitive component that handles basic toggle functionality
+ */
export interface TogglePrimitiveRootProps {
- defaultPressed? : boolean | false;
+ /** Initial pressed state for uncontrolled usage */
+ defaultPressed?: boolean;
+ /** Current pressed state for controlled usage. Takes precedence over defaultPressed */
pressed: boolean;
+ /** Content to be rendered within the toggle */
children?: React.ReactNode;
+ /** Additional CSS classes */
className?: string;
+ /** Callback fired when toggle state changes */
onChange : (isPressed:boolean) => 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.
| export interface TogglePrimitiveRootProps { | |
| defaultPressed? : boolean | false; | |
| pressed: boolean; | |
| children?: React.ReactNode; | |
| className?: string; | |
| onChange : (isPressed:boolean) => void; | |
| } | |
| /** | |
| * Props for the Toggle Primitive component that handles basic toggle functionality | |
| */ | |
| export interface TogglePrimitiveRootProps { | |
| /** Initial pressed state for uncontrolled usage */ | |
| defaultPressed?: boolean; | |
| /** Current pressed state for controlled usage. Takes precedence over defaultPressed */ | |
| pressed: boolean; | |
| /** Content to be rendered within the toggle */ | |
| children?: React.ReactNode; | |
| /** Additional CSS classes */ | |
| className?: string; | |
| /** Callback fired when toggle state changes */ | |
| onChange : (isPressed:boolean) => void; | |
| } |
| const TogglePrimitiveRoot = ({children,className='',defaultPressed,pressed,onChange,...props}:TogglePrimitiveRootProps) => { | ||
| const [isPressed, setIsPressed] = useState(pressed || defaultPressed); |
There was a problem hiding this comment.
Fix state management pattern
The current state management mixes controlled and uncontrolled patterns incorrectly:
-const TogglePrimitiveRoot = ({children,className='',defaultPressed,pressed,onChange,...props}:TogglePrimitiveRootProps) => {
- const [isPressed, setIsPressed] = useState(pressed || defaultPressed);
+const TogglePrimitiveRoot = ({
+ children,
+ className = '',
+ defaultPressed = false,
+ pressed,
+ onChange,
+ ...props
+}: TogglePrimitiveRootProps) => {
+ const [uncontrolledPressed, setUncontrolledPressed] = useState(defaultPressed);
+ const isPressed = pressed ?? uncontrolledPressed;📝 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 TogglePrimitiveRoot = ({children,className='',defaultPressed,pressed,onChange,...props}:TogglePrimitiveRootProps) => { | |
| const [isPressed, setIsPressed] = useState(pressed || defaultPressed); | |
| const TogglePrimitiveRoot = ({ | |
| children, | |
| className = '', | |
| defaultPressed = false, | |
| pressed, | |
| onChange, | |
| ...props | |
| }: TogglePrimitiveRootProps) => { | |
| const [uncontrolledPressed, setUncontrolledPressed] = useState(defaultPressed); | |
| const isPressed = pressed ?? uncontrolledPressed; |
| const handlePressed = () => { | ||
| const updatedPressed = !isPressed; | ||
| setIsPressed(updatedPressed); | ||
| onChange(updatedPressed) | ||
| } |
There was a problem hiding this comment.
Improve event handling
The toggle needs proper event handling for both mouse and keyboard interactions:
- const handlePressed = () => {
+ const handlePressed = (event: React.MouseEvent | React.KeyboardEvent) => {
+ if (
+ event.type === 'keydown' &&
+ (event as React.KeyboardEvent).key !== 'Enter' &&
+ (event as React.KeyboardEvent).key !== ' '
+ ) {
+ return;
+ }
+ event.preventDefault();
const updatedPressed = !isPressed;
- setIsPressed(updatedPressed);
+ if (pressed === undefined) {
+ setUncontrolledPressed(updatedPressed);
+ }
onChange(updatedPressed);
}📝 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 handlePressed = () => { | |
| const updatedPressed = !isPressed; | |
| setIsPressed(updatedPressed); | |
| onChange(updatedPressed) | |
| } | |
| const handlePressed = (event: React.MouseEvent | React.KeyboardEvent) => { | |
| if ( | |
| event.type === 'keydown' && | |
| (event as React.KeyboardEvent).key !== 'Enter' && | |
| (event as React.KeyboardEvent).key !== ' ' | |
| ) { | |
| return; | |
| } | |
| event.preventDefault(); | |
| const updatedPressed = !isPressed; | |
| if (pressed === undefined) { | |
| setUncontrolledPressed(updatedPressed); | |
| } | |
| onChange(updatedPressed); | |
| } |
I have added toggle primitive, context and stories file but their is no usage of toggle primitive in the toggle component, can you please suggest how to change the toggle component based on toggle primitive?
Summary by CodeRabbit
New Features
Togglecomponent with an optionaldefaultPressedproperty for initial state configuration.TogglePrimitiveRootcomponent to encapsulate toggle logic and state management.Improvements
aria-pressedattributes.Documentation
TogglePrimitivecomponent in Storybook for easier visualization and testing.