Refactor Accordion component with Collapsible Primitive and RovingFocusGroup #950
Conversation
WalkthroughThis update modifies the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 2
🧹 Nitpick comments (3)
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (1)
70-70: Unused variable for forced reflowThe
_variable is assigned but never used. This is intentional to force a browser reflow, but it's better to make this explicit to prevent confusion.-const _ = ref.current?.offsetHeight; +// Force a reflow by reading offsetHeight +void ref.current?.offsetHeight;Also applies to: 98-98
🧰 Tools
🪛 GitHub Check: lint
[warning] 70-70:
'_' is assigned a value but never usedsrc/components/ui/Accordion/fragments/AccordionRoot.tsx (2)
13-14: Optional transition props.
AddingtransitionDurationanddirectionbroadens the component’s flexibility. Be sure to document these new props in your story or docs.
34-38: Focus group usage with ref.
The pipeline error indicates that forwarding thereftoRovingFocusGroup.Groupis invalid. Integrate the fix noted above to avoid the runtime or type error.Would you like a new PR or patch with the updated code snippet to fix this issue?
🧰 Tools
🪛 GitHub Actions: Build RAD UI
[error] 35-35: Type '{ children: ReactNode; className: string; ref: MutableRefObject<HTMLDivElement | null>; }' is not assignable to type 'IntrinsicAttributes & { children: ReactNode; 'aria-label'?: string | undefined; 'aria-labelledby'?: string | undefined; } & HTMLAttributes'. Property 'ref' does not exist on type 'IntrinsicAttributes & { children: ReactNode; 'aria-label'?: string | undefined; 'aria-labelledby'?: string | undefined; } & HTMLAttributes'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/components/ui/Accordion/contexts/AccordionContext.tsx(2 hunks)src/components/ui/Accordion/fragments/AccordionContent.tsx(3 hunks)src/components/ui/Accordion/fragments/AccordionHeader.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionItem.tsx(3 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionTrigger.tsx(2 hunks)src/components/ui/Accordion/stories/Accordion.stories.tsx(3 hunks)src/components/ui/Avatar/Avatar.tsx(1 hunks)src/components/ui/Avatar/tests/Avatar.test.tsx(1 hunks)src/components/ui/Kbd/Kbd.tsx(1 hunks)src/components/ui/Kbd/stories/Kbd.stories.tsx(2 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx(3 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveTrigger.tsx(2 hunks)src/core/primitives/Primitive/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
src/components/ui/Accordion/fragments/AccordionHeader.tsx (1)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1) (1)
AccordionContext(15-25)
src/components/ui/Accordion/fragments/AccordionTrigger.tsx (2)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1) (1)
AccordionContext(15-25)src/components/ui/Accordion/contexts/AccordionItemContext.tsx (1) (1)
AccordionItemContext(11-17)
src/components/ui/Accordion/fragments/AccordionItem.tsx (2)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1) (1)
AccordionContext(15-25)src/components/ui/Accordion/contexts/AccordionItemContext.tsx (1) (1)
AccordionItemContext(11-17)
src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1) (1)
AccordionContext(15-25)
🪛 GitHub Check: lint
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx
[warning] 98-98:
'_' is assigned a value but never used
[warning] 70-70:
'_' is assigned a value but never used
🪛 GitHub Actions: Build RAD UI
src/components/ui/Accordion/fragments/AccordionRoot.tsx
[error] 27-27: Type '{ rootClass: string; activeItem: number | null; setActiveItem: Dispatch<SetStateAction<number | null>>; accordionRef: MutableRefObject<HTMLDivElement | null>; transitionDuration: number; }' is missing the following properties from type 'AccordionContextType': setFocusItem, focusNextItem, focusPrevItem.
[error] 35-35: Type '{ children: ReactNode; className: string; ref: MutableRefObject<HTMLDivElement | null>; }' is not assignable to type 'IntrinsicAttributes & { children: ReactNode; 'aria-label'?: string | undefined; 'aria-labelledby'?: string | undefined; } & HTMLAttributes'. Property 'ref' does not exist on type 'IntrinsicAttributes & { children: ReactNode; 'aria-label'?: string | undefined; 'aria-labelledby'?: string | undefined; } & HTMLAttributes'.
🔇 Additional comments (35)
src/components/ui/Avatar/tests/Avatar.test.tsx (1)
29-29: Clean code improvement: Added a blank line for better test separation.The added blank line improves readability by clearly separating test cases.
src/components/ui/Avatar/Avatar.tsx (1)
26-26: Formatting improvement: Removed extra space before spread operator.This minor formatting change improves consistency in JSX attribute spacing.
src/components/ui/Kbd/Kbd.tsx (2)
16-16: Formatting improvement: Added consistent spacing around equals sign.The default value assignment for the
sizeparameter now has proper spacing, which improves code style consistency.
18-19: Formatting improvement: Adjusted line spacing.Improved spacing around the
dataAttributesdeclaration for better code style consistency.src/components/ui/Accordion/fragments/AccordionHeader.tsx (3)
1-1: Good refactoring: Added necessary imports for context usage.Added imports for
useContextandAccordionContextto properly implement context-based styling.Also applies to: 3-3
11-11: Good pattern: Added context usage to retrieve rootClass.Properly uses the React
useContexthook to access therootClassfromAccordionContext.
13-13: Improved styling consistency: Using rootClass for header styling.By using the
rootClassfrom context to create the component's class name structure, this change ensures consistent styling patterns across accordion components. This aligns with the PR's objective of refactoring the Accordion components.src/components/ui/Accordion/fragments/AccordionContent.tsx (3)
6-7: Good integration of CollapsiblePrimitiveAdding the CollapsiblePrimitive import supports the accordion refactoring mentioned in the PR objectives. This moves the component toward a more composable architecture.
24-26: Appropriate use of CollapsiblePrimitive.Content with asChild propThe replacement of a div element with CollapsiblePrimitive.Content is a good architectural decision. The asChild prop enables polymorphic rendering, allowing the component to maintain its semantics while leveraging the collapsible functionality.
35-35: Consistent closing tagThe closing tag properly matches the opening CollapsiblePrimitive.Content element.
src/components/ui/Accordion/contexts/AccordionContext.tsx (2)
12-12: Good addition of transitionDuration propertyAdding the optional transitionDuration property to the AccordionContextType interface enhances customization capabilities. This aligns with the PR goal of refactoring the Accordion component with better primitives.
23-24: Appropriate default values for contextSetting a default value of 0 for transitionDuration ensures the component works correctly even when the property isn't explicitly provided.
src/components/ui/Kbd/stories/Kbd.stories.tsx (2)
5-5: Minor syntax improvementAdding the semicolon maintains consistent coding style.
32-36: Improved code formattingThe indentation changes improve readability and make the component structure clearer.
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveTrigger.tsx (2)
24-24: Removed className from props destructuringThe removal of className from the function parameters aligns with the changes in how styles are applied to this component. This is part of moving toward a more consistent component pattern across the library.
43-43: Good use of asChild prop for polymorphic supportUsing the asChild prop directly in the Primitive.button component enhances the component's flexibility, allowing it to adapt its rendering based on the context. This is consistent with modern React component design patterns and supports the refactoring goals.
src/core/primitives/Primitive/index.tsx (1)
18-52: Enhanced asChild handling with improved ref management 👍This is a solid improvement to the asChild implementation that addresses several edge cases:
- Better validation ensuring exactly one valid child element
- Proper warning message when validation fails
- Improved ref merging that correctly handles both the forwarded ref and the child's original ref
The approach of prioritizing the child's props over elementProps while ensuring refs are properly merged is the correct pattern for polymorphic components.
src/components/ui/Accordion/fragments/AccordionTrigger.tsx (3)
2-7: Good addition of necessary imports for refactoringThese new imports support the shift to a more structured approach with CollapsiblePrimitive and RovingFocusGroup.
18-20: Simplified context usage and added refThe addition of triggerRef and simplified context usage supports the removal of complex event handlers in favor of the more structured approach with the new primitives.
30-44: Structural refactoring for enhanced focus management and collapsible behaviorThis refactoring wraps the button element with RovingFocusGroup.Item and CollapsiblePrimitive.Trigger, providing more structured focus management and collapsible behavior.
Key improvements:
- Uses a more declarative approach instead of imperative event handlers
- Leverages the asChild pattern for proper prop forwarding
- Maintains accessibility with appropriate aria attributes
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (8)
42-42: Added rafRef for better animation frame managementThe addition of a RequestAnimationFrame reference improves animation handling by providing a way to track and cancel animation frames.
50-53: Added cleanup for animation framesProperly cleaning up animation frames prevents conflicts and potential memory leaks.
57-60: Added special handling for zero transition durationThis optimization bypasses animation logic when transitionDuration is 0, improving performance by directly setting the final state.
63-84: Improved open animation sequenceThe enhanced open animation logic ensures smoother transitions by:
- Setting height to 0 first if it was undefined
- Using RAF to properly sequence measurements and updates
- More accurately measuring content height
🧰 Tools
🪛 GitHub Check: lint
[warning] 70-70:
'_' is assigned a value but never used
91-104: Enhanced close animation logicSetting the height to the current content height before animating to 0 ensures a smooth start to the closing animation.
🧰 Tools
🪛 GitHub Check: lint
[warning] 98-98:
'_' is assigned a value but never used
111-113: Added cleanup for animation frames in useEffect returnEnsures proper cleanup of animation frames when the component unmounts or when dependencies change.
115-115: Updated useEffect dependenciesAdding height to the dependency array ensures the effect runs when height changes, maintaining animation consistency.
127-129: Conditional transition stylesApplying transition styles only when transitionDuration > 0 maintains clean styling when no animation is desired.
src/components/ui/Accordion/stories/Accordion.stories.tsx (3)
24-65: Well-structured accordion items dataThe items array has been nicely formatted for improved readability while maintaining the same content.
68-70: Enhanced AccordionExample with props forwardingThe AccordionExample component now accepts and forwards props to the Accordion.Root, making it more flexible and reusable.
92-94: Added animation example storyThe new WithAnimation story showcases the transitionDuration prop functionality, providing a good example of the animation capabilities.
src/components/ui/Accordion/fragments/AccordionItem.tsx (3)
6-6: Great addition of CollapsiblePrimitive.
Importing theCollapsiblePrimitivealigns well with the new refactoring approach. This makes the collapsible behavior more declarative and reusable.
18-18: Use oftransitionDurationfrom context.
RetrievingtransitionDurationhere is consistent with the updated context approach. Ensure that places using this component handle the optional nature of this prop, particularly if zero is intended to disable transitions.If you rely on a zero value to disable transitions, please confirm no animations or side effects occur in Collapsible at zero duration. If needed, we can add a safeguard check.
32-43: Smooth integration of CollapsiblePrimitive.
Wrapping the content withCollapsiblePrimitive.Rootand usingasChildis a solid way to maintain the styling. No issues found, but consider verifying that nested focus or event handling (withinchildren) behaves as expected post-refactor.src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
17-17: Default prop assignments.
Providingdirection = 'vertical'andtransitionDuration = 0is a good way to fall back to existing logic.
| accordionRef, | ||
| transitionDuration |
There was a problem hiding this comment.
Pipeline error regarding missing context props.
Your AccordionContextType still includes focusItem, setFocusItem, focusNextItem, and focusPrevItem, but they’re not being provided here. This triggers the type mismatch in the pipeline.
To resolve:
- If these focus props are truly no longer needed, remove them from the
AccordionContextType. - Otherwise, supply placeholder no-op functions (or actual implementations) while you finalize refactoring.
value={{
rootClass,
activeItem,
setActiveItem,
accordionRef,
+ // If you still need them, add them here or remove them from the interface:
+ // focusItem: null,
+ // setFocusItem: () => {},
+ // focusNextItem: () => {},
+ // focusPrevItem: () => {},
transitionDuration
}}📝 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.
| accordionRef, | |
| transitionDuration | |
| value={{ | |
| rootClass, | |
| activeItem, | |
| setActiveItem, | |
| accordionRef, | |
| // If you still need them, add them here or remove them from the interface: | |
| // focusItem: null, | |
| // setFocusItem: () => {}, | |
| // focusNextItem: () => {}, | |
| // focusPrevItem: () => {}, | |
| transitionDuration | |
| }} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx (1)
54-54: Consider the UX impact of changing the timing functionThe default timing function has been changed from 'ease-out' to 'linear', which may affect the perceived smoothness of animations. While 'ease-out' creates a more natural feel by starting fast and slowing down, 'linear' maintains the same speed throughout.
Is this change intentional? If so, ensure it's consistent with other components in the system to maintain a unified animation experience. If not, consider reverting to 'ease-out' or documenting the rationale.
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (1)
83-83: Silence lint warnings about unused variablesThe static analysis tool is flagging unused
_variables, but these are intentionally used to force a browser reflow.Consider adding a comment to explain the purpose of these lines:
- const _ = ref.current?.offsetHeight; + // Force a reflow by reading offsetHeight before continuing + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const _ = ref.current?.offsetHeight;Also applies to: 107-107
🧰 Tools
🪛 GitHub Check: lint
[warning] 83-83:
'_' is assigned a value but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/ui/Accordion/contexts/AccordionContext.tsx(1 hunks)src/components/ui/Accordion/contexts/AccordionItemContext.tsx(1 hunks)src/components/ui/Accordion/fragments/AccordionItem.tsx(3 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(1 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx(2 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx(1 hunks)src/core/primitives/Primitive/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/primitives/Primitive/index.tsx
- src/components/ui/Accordion/fragments/AccordionItem.tsx
🧰 Additional context used
🧬 Code Definitions (1)
src/components/ui/Accordion/fragments/AccordionRoot.tsx (1)
src/components/ui/Accordion/contexts/AccordionContext.tsx (1) (1)
AccordionContext(12-19)
🪛 GitHub Check: lint
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx
[warning] 107-107:
'_' is assigned a value but never used
[warning] 83-83:
'_' is assigned a value but never used
🔇 Additional comments (17)
src/components/ui/Accordion/contexts/AccordionItemContext.tsx (1)
3-11: Clean simplification of the AccordionItemContextThe removal of event handler properties (
handleBlurEvent,handleClickEvent, andhandleFocusEvent) from the context is a good simplification. This aligns with the PR's goal of using RovingFocusGroup for focus management.src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (8)
41-44: LGTM: Improved conditional renderingAdding the
shouldRenderstate to control component rendering based on open state is a good optimization that prevents unnecessary DOM operations.
45-51: Proper handling of open state transitionsThis useEffect correctly ensures the component immediately renders when opened, which is essential for smooth animations.
52-74: Good enhancement: Zero-duration transition handlingAdding special handling for when
transitionDurationis 0 makes the component more robust. This allows for immediate state changes without animation when desired.
75-93: Improved animation sequencing for opening stateThe enhanced opening animation with proper sequencing using requestAnimationFrame ensures smooth transitions without layout thrashing.
🧰 Tools
🪛 GitHub Check: lint
[warning] 83-83:
'_' is assigned a value but never used
99-119: Improved animation sequencing for closing stateThe enhanced closing animation ensures a smooth transition by first capturing the current height, then animating to zero.
🧰 Tools
🪛 GitHub Check: lint
[warning] 107-107:
'_' is assigned a value but never used
125-128: Good practice: Proper cleanup of animation framesAdding cleanup for animation frames prevents memory leaks and ensures animations don't conflict when component state changes rapidly.
131-134: Optimization: Conditional renderingNot rendering the component when it's closed and animation is complete is an excellent performance optimization.
146-149: Conditional transition styleOnly applying the transition style when duration > 0 is a good practice that prevents unnecessary style calculations.
src/components/ui/Accordion/contexts/AccordionContext.tsx (2)
7-10: Good refactor: Simplified context with new transition propertiesRemoving focus management properties (
focusItem,setFocusItem,focusNextItem,focusPrevItem) and adding transition control properties is a clean simplification that aligns with using RovingFocusGroup.
16-19:❓ Verification inconclusive
Note timing function inconsistency across components
The default timing function here is 'ease-out', but in CollapsiblePrimitiveRoot it was changed to 'linear'.
Verify if this inconsistency is intentional. If not, consider aligning the default values across components:
🏁 Script executed:
#!/bin/bash # Check for other timing function defaults in the codebase echo "Searching for transition timing function defaults in the codebase..." rg -i "transitionTimingFunction\s*=\s*['\"]" --type=tsx --type=tsLength of output: 244
Action Required: Confirm Timing Function Defaults Consistency
The default timing function in
src/components/ui/Accordion/contexts/AccordionContext.tsxis set to'ease-out', yet elsewhere (inCollapsiblePrimitiveRoot) it appears to have been changed to'linear'. The initial search command returned an error due to file type handling, so please run the updated script above to help verify if other components indeed use a different default value. If this discrepancy is unintentional, consider aligning the defaults across components.
🏁 Script executed:
#!/bin/bash echo "Searching for transition timing function defaults in .ts and .tsx files..." rg -i "transitionTimingFunction\s*=\s*['\"]" --glob "*.ts" --glob "*.tsx"Length of output: 864
Inconsistent Timing Function Default
Our recent search shows that whileAccordionContext.tsxcurrently sets the defaulttransitionTimingFunctionto'ease-out', bothCollapsiblePrimitiveRoot.tsxandAccordionRoot.tsxare using'linear'. Please confirm whether this discrepancy is intentional. If it’s not, please update the default inAccordionContext.tsx(lines 16–19) to'linear'for consistency.src/components/ui/Accordion/fragments/AccordionRoot.tsx (6)
5-6: Great addition of RovingFocusGroup for improved accessibility.Using the RovingFocusGroup component is an excellent approach for implementing keyboard navigation and focus management in the accordion, which significantly improves accessibility.
13-15: Good API expansion with transition and direction controls.Adding these optional props provides developers with more flexibility to customize the accordion's behavior and appearance. This is a good enhancement to the component's API.
18-18: Appropriate defaults for the new props.The default values chosen for direction ('vertical'), transitionDuration (0), and transitionTimingFunction ('linear') make sense as they provide sensible fallbacks while maintaining backward compatibility.
30-32: Context value correctly updated.The focus management properties have been properly removed from the context value, and the new transition properties have been added, aligning with the updated AccordionContextType interface.
34-40: Proper implementation of RovingFocusGroup with ref handling.The implementation correctly addresses the previous pipeline error by separating the ref handling from the RovingFocusGroup component, placing it on an inner div element. This resolves the type incompatibility issue while maintaining the desired functionality.
24-43:❓ Verification inconclusive
Verify transition properties propagation to child components.
The transition properties are now correctly provided through context, but it's important to ensure that the child components (AccordionItem, AccordionTrigger, AccordionContent) are properly consuming these properties to apply the transitions.
🏁 Script executed:
#!/bin/bash # Check if the child components are using the transition properties from context echo "Checking AccordionContent.tsx for transition properties usage:" rg -A 3 "transitionDuration|transitionTimingFunction" --type tsx src/components/ui/Accordion/fragments/AccordionContent.tsx || echo "No transition properties found in AccordionContent.tsx" echo "Checking AccordionTrigger.tsx for transition properties usage:" rg -A 3 "transitionDuration|transitionTimingFunction" --type tsx src/components/ui/Accordion/fragments/AccordionTrigger.tsx || echo "No transition properties found in AccordionTrigger.tsx" echo "Checking for CollapsiblePrimitive usage which might be handling transitions:" rg -A 3 "Collapsible" --type tsx src/components/ui/Accordion/fragments/ || echo "No CollapsiblePrimitive found"Length of output: 1147
Action Required: Verify Transition Property Consumption in Child Components
The AccordionRoot now correctly provides the transition properties via context. However, our initial search did not reveal explicit references to
transitionDurationortransitionTimingFunctionin AccordionContent.tsx and AccordionTrigger.tsx, nor did it show any intermediary usage by a component like CollapsiblePrimitive.
- Verify Consumption: Please check that both AccordionContent and AccordionTrigger (or any other intermediary component) are consuming AccordionContext (e.g., via
useContext(AccordionContext)) and applying the transition properties in their styling or behavior.- Update as Needed: If the children aren’t yet utilizing these properties, consider updating their implementation so that the transition values drive the intended animations.
Summary by CodeRabbit
New Features
Refactor
asChildprop for improved component robustness.