Collapsible Primitive #947
Conversation
WalkthroughThis pull request introduces a comprehensive set of collapsible component primitives. It adds a new context with its associated hook and types, along with React components for the collapsible root, content, and trigger. An aggregator module re-exports these primitives, and accompanying Storybook stories and tests demonstrate and verify usage scenarios, state management, and accessibility through ARIA attributes. Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Trigger
participant CTX as Context
participant RT as Root
participant CNT as Content
TR->>CTX: useCollapsiblePrimitiveContext()
CTX-->>TR: Return open state & onOpenChange
TR->>CTX: onOpenChange (triggered by click)
CTX->>RT: Update open/closed state
RT->>CNT: Propagate updated state via context
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (5)
src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (1)
1-1: Remove unused React importThe React import isn't directly used in this file as there's no JSX being rendered. While it's commonly imported by habit, it's better to remove unused imports.
-import React, { createContext, useContext } from 'react'; +import { createContext, useContext } from 'react';🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'React' is defined but never usedsrc/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (1)
56-94: Well-implemented animation logic with proper cleanupThe animation logic is thorough and handles both opening and closing states with proper timing. The component:
- Correctly manages height transitions
- Uses
requestAnimationFramefor smooth animations- Properly cleans up timeouts to prevent memory leaks
- Sets height to
undefinedafter opening for responsive flexibilityOne improvement could be made in the forced reflow implementation:
// Line 80: Use a more explicit variable name to indicate purpose - const _ = ref.current?.offsetHeight; + // Force a reflow by reading offsetHeight + void ref.current?.offsetHeight;🧰 Tools
🪛 GitHub Check: lint
[warning] 80-80:
'_' is assigned a value but never usedsrc/core/primitives/Collapsible/stories/Collapsible.stories.tsx (2)
36-36: Consider removing the potentially unnecessary data attribute.The
data-radui-collapsible-contentattribute appears to be a leftover from a dependency or previous implementation. If it's not being used for styling or selection, it should be removed to keep the code clean.- data-radui-collapsible-content
168-172: Mixing animation properties and manual display styling can create conflicts.Using both
transitionDuration={0}and manually controlling display withstyle={{display: isOpen ? 'block' : 'none'}}is redundant and could lead to unexpected behavior. The component likely handles visibility internally when animation is disabled.<CollapsiblePrimitive.Content transitionDuration={0} - style={{ - display: isOpen ? 'block' : 'none', - height: 'auto' - }} >src/core/primitives/Collapsible/tests/Collapsible.test.tsx (1)
70-93: Consider enhancing the controlled component test.While the test correctly verifies that the
onOpenChangecallback is called, it doesn't fully test the component behavior in controlled mode. Consider adding a test that simulates the parent updating theopenprop in response to the callback.test('onOpenChange callback is called when trigger is clicked', () => { const onOpenChangeMock = jest.fn(); + const { rerender } = render( - render( <CollapsiblePrimitive.Root onOpenChange={onOpenChangeMock}> <CollapsiblePrimitive.Trigger data-testid="trigger"> Toggle </CollapsiblePrimitive.Trigger> <CollapsiblePrimitive.Content data-testid="content"> <div>Content</div> </CollapsiblePrimitive.Content> </CollapsiblePrimitive.Root> ); // Initial state expect(screen.getByTestId('content')).toHaveAttribute('data-state', 'closed'); // Toggle open fireEvent.click(screen.getByTestId('trigger')); expect(onOpenChangeMock).toHaveBeenCalledWith(true); + // Simulate parent updating the state + rerender( + <CollapsiblePrimitive.Root open={true} onOpenChange={onOpenChangeMock}> + <CollapsiblePrimitive.Trigger data-testid="trigger"> + Toggle + </CollapsiblePrimitive.Trigger> + <CollapsiblePrimitive.Content data-testid="content"> + <div>Content</div> + </CollapsiblePrimitive.Content> + </CollapsiblePrimitive.Root> + ); + + // Verify content is now open + expect(screen.getByTestId('content')).toHaveAttribute('data-state', 'open'); + + // Toggle closed + fireEvent.click(screen.getByTestId('trigger')); + expect(onOpenChangeMock).toHaveBeenCalledWith(false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx(1 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx(1 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx(1 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveTrigger.tsx(1 hunks)src/core/primitives/Collapsible/index.tsx(1 hunks)src/core/primitives/Collapsible/stories/Collapsible.stories.tsx(1 hunks)src/core/primitives/Collapsible/tests/Collapsible.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveTrigger.tsx (2)
src/core/primitives/Collapsible/index.tsx (1) (1)
CollapsiblePrimitiveTriggerProps(9-9)src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (1) (1)
useCollapsiblePrimitiveContext(24-34)
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx (2)
src/core/primitives/Collapsible/index.tsx (1) (1)
CollapsiblePrimitiveRootProps(7-7)src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (1) (1)
CollapsiblePrimitiveContext(22-22)
🪛 GitHub Check: lint
src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx
[warning] 1-1:
'React' is defined but never used
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx
[warning] 80-80:
'_' is assigned a value but never used
🔇 Additional comments (12)
src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (2)
3-20: Well-structured context type definition with thorough documentationThe
CollapsiblePrimitiveContextValuetype is well-defined with clear JSDoc comments explaining each property. The inclusion of thecontentIdfor ARIA relationships shows good consideration for accessibility.
24-34: Good error handling in the context hookThe
useCollapsiblePrimitiveContexthook properly checks if the context exists before returning it, providing a helpful error message that guides developers on correct usage within the component hierarchy.src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx (2)
6-37: Well-documented props with comprehensive type definitionThe
CollapsiblePrimitiveRootPropstype is thoroughly documented with clear JSDoc comments for each property. The inclusion of both controlled and uncontrolled patterns throughopenanddefaultOpenprops follows React best practices.
39-79: Well-implemented component with proper state managementThe component correctly:
- Uses
useControllableStatefor managing controlled/uncontrolled state- Handles the disabled state appropriately
- Provides complete context values to children
- Uses data attributes for styling hooks
The implementation of
handleOpenChangecorrectly prevents state changes when the component is disabled.src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveTrigger.tsx (2)
27-55: Excellent accessibility implementation with proper ARIA attributesThe component correctly implements accessibility features with:
aria-controlslinked to the content IDaria-expandedreflecting the open state- Data attributes for styling based on component state
- Proper ref forwarding for parent component access
- Clean event handling that allows event propagation
This implementation follows best practices for accessible UI components.
31-37: Clean event handler implementationThe
handleClickfunction is well implemented, allowing event propagation while still toggling the open state. The disabled check prevents state changes when appropriate.src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (2)
37-55: Good implementation of prop override for context valuesThe component allows the
openprop to override the context value, providing flexibility while maintaining the automatic context behavior when the prop is omitted. The default values for animation settings are sensible.
96-112: Properly implemented accessibility attributes with clean stylingThe component correctly implements:
idlinked to the trigger'saria-controlsaria-hiddenattribute reflecting the open state- Clean inline style management for animations
- Data attributes for styling hooks
The transition styling is applied efficiently with dynamic values based on props.
src/core/primitives/Collapsible/stories/Collapsible.stories.tsx (1)
1-222: Well-structured Storybook examples with comprehensive coverage.The stories provide excellent examples of different usage patterns for the CollapsiblePrimitive component, including uncontrolled state, controlled state, multiple instances, and animation customization. This will serve as good documentation for consumers of the component.
src/core/primitives/Collapsible/index.tsx (2)
17-20: Good implementation of compound component pattern with helpful warning.The warning message is helpful for guiding developers to use the correct sub-components instead of the main component directly. This follows best practices seen in other React component libraries.
1-28: Clean compound component implementation with proper type exports.The file follows best practices for implementing compound components in React:
- Re-exporting prop types for type safety
- Using the direct assignment pattern for sub-components
- Providing clear guidance for usage
This approach makes the component easy to use and provides good developer experience.
src/core/primitives/Collapsible/tests/Collapsible.test.tsx (1)
1-125: Comprehensive test coverage with good accessibility testing.The test suite covers all important aspects of the component:
- Rendering behavior
- State management
- Controlled and uncontrolled modes
- Event handling
- Accessibility attributes
This thorough testing approach ensures the component will work reliably and be accessible to all users.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx (3)
1-4: Consider removing unused default React import.The default import
Reactisn’t referenced directly, as only named exports (useId) are used. This can trigger lint warnings in some setups.-import React, { useId } from 'react'; +import { useId } from 'react';
6-45: Comprehensive prop documentation.The
CollapsiblePrimitiveRootPropsinterface is clearly documented, which helps maintainers understand each prop’s usage. Note that[key: string]: any;is flexible but can potentially mask type errors. You might consider a more specific approach for additional HTML attributes.
47-91: Implementation looks solid; consider adding ARIA attribute for disabled state.Providing
[data-disabled]is helpful for styling or logic, but screen readers may benefit fromaria-disabled="true"when disabled is true. Verify if this aligns with your accessibility goals.src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (1)
1-2: Remove unused default React import.Only named exports (
createContext,useContext) are used, so the defaultReactimport is unnecessary.-import React, { createContext, useContext } from 'react'; +import { createContext, useContext } from 'react';🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'React' is defined but never usedsrc/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (2)
5-22: Docstrings enhance clarity for component props.Using JSDoc on each prop is helpful for future maintainers and tooling. Similar to the root props, consider refining
[key: string]: any;if more specific typing is preferred.
43-81: Good use of forced reflow; consider renaming the underscore.Setting a temporary variable (currently
_) triggers layout calculation to ensure the animation transitions smoothly. Renaming_to_forceReflowor similar might clarify its intent.🧰 Tools
🪛 GitHub Check: lint
[warning] 67-67:
'_' is assigned a value but never usedsrc/core/primitives/Collapsible/stories/Collapsible.stories.tsx (2)
16-46: Consider removing the data attribute that seems out of place.The story demonstrates a basic collapsible implementation well, but there's an unnecessary
data-radui-collapsible-contentattribute on line 36 that doesn't appear in other stories and seems like a remnant from Radix UI.<CollapsiblePrimitive.Content - data-radui-collapsible-content >
48-83: Controlled example is well-implemented with a minor formatting issue.The controlled component pattern is properly demonstrated with state management. The comment on line 50 has inconsistent indentation with the surrounding code.
- // Use React state for controlled component + // Use React state for controlled component
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx(1 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx(1 hunks)src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx(1 hunks)src/core/primitives/Collapsible/stories/Collapsible.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveRoot.tsx (2)
src/core/primitives/Collapsible/index.tsx (1) (1)
CollapsiblePrimitiveRootProps(7-7)src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (1) (1)
CollapsiblePrimitiveContext(38-38)
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (2)
src/core/primitives/Collapsible/index.tsx (1) (1)
CollapsiblePrimitiveContentProps(8-8)src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (1) (1)
useCollapsiblePrimitiveContext(40-50)
🪛 GitHub Check: lint
src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx
[warning] 1-1:
'React' is defined but never used
src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx
[warning] 67-67:
'_' is assigned a value but never used
🔇 Additional comments (10)
src/core/primitives/Collapsible/contexts/CollapsiblePrimitiveContext.tsx (2)
3-36: Well-structured context type.The properties and JSDoc clearly convey the context value’s purpose, enhancing maintainability and discoverability.
38-50: Custom hook usage is appropriate.Requiring usage within
CollapsiblePrimitiveRootensures consistent state management, which is a best practice for compound components.src/core/primitives/Collapsible/fragments/CollapsiblePrimitiveContent.tsx (5)
1-3: Imports are valid for forwardRef and hooks usage.The default
Reactimport is needed forReact.forwardRef. No concerns here.
24-36: ForwardRef pattern is well structured.Wrapping the component in
React.forwardRefensures consistent ref handling across parent components.
38-42: Ref combination is done properly.Assigning the forwarded ref to
combinedRefextends the usage while allowing fallback to a local ref. This is a clean approach.
83-99: Transition logic and ARIA usage look correct.Dynamically setting height and
aria-hiddenaligns with best practices for collapsible content. This should support screen readers appropriately.
103-105: DisplayName and default export assigned properly.Providing
displayNameimproves debugging in React DevTools. The default export keeps the component import succinct.src/core/primitives/Collapsible/stories/Collapsible.stories.tsx (3)
1-15: Well-structured Storybook setup!The imports and metadata are correctly set up following Storybook's best practices. The component is properly categorized under "Primitives" with appropriate parameters.
85-141: Excellent implementation of multiple collapsibles.This example effectively demonstrates managing multiple independent collapsible elements using a record for state management. The approach of mapping over Object.entries with unique keys is clean and efficient.
184-220: Custom animation implementation showcases the headless nature well.This example effectively demonstrates how to customize animations through props rather than inline styles, correctly following the headless UI pattern. The cubic-bezier timing function creates a nice spring effect, and the explanation in the content helps users understand the approach.
| export const AnimationDisabled: Story = { | ||
| render: () => { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
|
|
||
| return ( | ||
| <div style={{ width: '300px', border: '1px solid #ccc', borderRadius: '4px', padding: '8px' }}> | ||
| <CollapsiblePrimitive.Root | ||
| open={isOpen} | ||
| onOpenChange={(open) => setIsOpen(open)} | ||
| transitionDuration={0} | ||
| > | ||
| <div style={{ display: 'flex', justifyContent: 'space-between', alignItems: 'center', padding: '8px' }}> | ||
| <span>No Animation</span> | ||
| <CollapsiblePrimitive.Trigger | ||
| style={{ | ||
| background: 'none', | ||
| border: '1px solid #ddd', | ||
| borderRadius: '4px', | ||
| padding: '4px 8px', | ||
| cursor: 'pointer' | ||
| }} | ||
| > | ||
| {isOpen ? 'Close' : 'Open'} | ||
| </CollapsiblePrimitive.Trigger> | ||
| </div> | ||
| <CollapsiblePrimitive.Content | ||
| style={{ | ||
| display: isOpen ? 'block' : 'none', | ||
| height: 'auto' | ||
| }} | ||
| > | ||
| <div style={{ padding: '8px', backgroundColor: '#f5f5f5', borderRadius: '4px', marginTop: '8px' }}> | ||
| <p>This content toggles instantly without animation.</p> | ||
| </div> | ||
| </CollapsiblePrimitive.Content> | ||
| </CollapsiblePrimitive.Root> | ||
| </div> | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potentially conflicting animation control approaches.
While setting transitionDuration={0} is the right way to disable animations, adding inline styles to directly control the Content's display and height properties (lines 169-172) might conflict with the component's internal behavior.
<CollapsiblePrimitive.Content
- style={{
- display: isOpen ? 'block' : 'none',
- height: 'auto'
- }}
>This change would let the component handle its own visibility, relying solely on the transitionDuration={0} prop to disable animations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const AnimationDisabled: Story = { | |
| render: () => { | |
| const [isOpen, setIsOpen] = useState(false); | |
| return ( | |
| <div style={{ width: '300px', border: '1px solid #ccc', borderRadius: '4px', padding: '8px' }}> | |
| <CollapsiblePrimitive.Root | |
| open={isOpen} | |
| onOpenChange={(open) => setIsOpen(open)} | |
| transitionDuration={0} | |
| > | |
| <div style={{ display: 'flex', justifyContent: 'space-between', alignItems: 'center', padding: '8px' }}> | |
| <span>No Animation</span> | |
| <CollapsiblePrimitive.Trigger | |
| style={{ | |
| background: 'none', | |
| border: '1px solid #ddd', | |
| borderRadius: '4px', | |
| padding: '4px 8px', | |
| cursor: 'pointer' | |
| }} | |
| > | |
| {isOpen ? 'Close' : 'Open'} | |
| </CollapsiblePrimitive.Trigger> | |
| </div> | |
| <CollapsiblePrimitive.Content | |
| style={{ | |
| display: isOpen ? 'block' : 'none', | |
| height: 'auto' | |
| }} | |
| > | |
| <div style={{ padding: '8px', backgroundColor: '#f5f5f5', borderRadius: '4px', marginTop: '8px' }}> | |
| <p>This content toggles instantly without animation.</p> | |
| </div> | |
| </CollapsiblePrimitive.Content> | |
| </CollapsiblePrimitive.Root> | |
| </div> | |
| ); | |
| } | |
| }; | |
| export const AnimationDisabled: Story = { | |
| render: () => { | |
| const [isOpen, setIsOpen] = useState(false); | |
| return ( | |
| <div style={{ width: '300px', border: '1px solid #ccc', borderRadius: '4px', padding: '8px' }}> | |
| <CollapsiblePrimitive.Root | |
| open={isOpen} | |
| onOpenChange={(open) => setIsOpen(open)} | |
| transitionDuration={0} | |
| > | |
| <div style={{ display: 'flex', justifyContent: 'space-between', alignItems: 'center', padding: '8px' }}> | |
| <span>No Animation</span> | |
| <CollapsiblePrimitive.Trigger | |
| style={{ | |
| background: 'none', | |
| border: '1px solid #ddd', | |
| borderRadius: '4px', | |
| padding: '4px 8px', | |
| cursor: 'pointer' | |
| }} | |
| > | |
| {isOpen ? 'Close' : 'Open'} | |
| </CollapsiblePrimitive.Trigger> | |
| </div> | |
| <CollapsiblePrimitive.Content> | |
| <div style={{ padding: '8px', backgroundColor: '#f5f5f5', borderRadius: '4px', marginTop: '8px' }}> | |
| <p>This content toggles instantly without animation.</p> | |
| </div> | |
| </CollapsiblePrimitive.Content> | |
| </CollapsiblePrimitive.Root> | |
| </div> | |
| ); | |
| } | |
| }; |
Summary by CodeRabbit
New Features
CollapsiblePrimitiveRoot,CollapsiblePrimitiveContent, andCollapsiblePrimitiveTrigger.Tests