Collapsible Improvements + Docs Improvements #1000
Conversation
WalkthroughThe pull request includes updates across stylesheets, component logic, linting scripts, and documentation examples. The changes simplify animation keyframe declarations and introduce a new blur effect style. The code block component now manages an expansion state with a toggle button. The linting configuration in package.json is updated to auto-fix style issues. Major refactors of the collapsible component and its fragments remove legacy functionality, delegate behavior to new primitives, and simplify associated stories and styling. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CB as CodeBlock
participant SC as State Manager
U->>CB: Clicks toggle button
CB->>SC: Toggle expanded state
SC-->>CB: Returns updated state
CB->>U: Re-renders code block (blur effect updated)
sequenceDiagram
participant U as User
participant CT as Collapsible.Trigger
participant CP as CollapsiblePrimitive
U->>CT: Clicks trigger
CT->>CP: Delegates collapse/expand handling
CP-->>CT: Returns updated state
CP->>Content: Renders updated collapsible content
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: 3
🧹 Nitpick comments (1)
docs/components/layout/Documentation/helpers/CodeBlock.js (1)
54-61: Consider conditionally rendering the expand button only when neededThe code block is always limited to 180px height when collapsed, but there's no check to see if the content actually requires collapsing. Consider only showing the expand button if content exceeds a certain height.
-{!expanded && <div className="code-block-blur"></div>} -<div className="flex justify-center w-full bg-gradient-to-t from-background to-transparent bg-gray-100 px-4 py-2"> - - <Button size="small" onClick={() => setExpanded(!expanded)}> - Show {expanded ? 'less' : 'more'} - </Button> -</div> +{codeHeight > 180 && ( + <> + {!expanded && <div className="code-block-blur"></div>} + <div className="flex justify-center w-full bg-gradient-to-t from-background to-transparent bg-gray-100 px-4 py-2"> + <Button size="small" onClick={() => setExpanded(!expanded)}> + Show {expanded ? 'less' : 'more'} + </Button> + </div> + </> +)}You would need to measure the content height using a ref:
const codeRef = useRef(null); const [codeHeight, setCodeHeight] = useState(0); useEffect(() => { if (codeRef.current) { setCodeHeight(codeRef.current.scrollHeight); } }, [children]); // Then add ref={codeRef} to the code element
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/app/globals.scss(1 hunks)docs/components/layout/Documentation/helpers/CodeBlock.js(3 hunks)package.json(1 hunks)src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/fragments/CollapsibleContent.tsx(2 hunks)src/components/ui/Collapsible/fragments/CollapsibleHeader.tsx(0 hunks)src/components/ui/Collapsible/fragments/CollapsibleItem.tsx(0 hunks)src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx(2 hunks)src/components/ui/Collapsible/index.tsx(0 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(2 hunks)styles/themes/components/collapsible.scss(1 hunks)
💤 Files with no reviewable changes (3)
- src/components/ui/Collapsible/fragments/CollapsibleHeader.tsx
- src/components/ui/Collapsible/fragments/CollapsibleItem.tsx
- src/components/ui/Collapsible/index.tsx
🧰 Additional context used
🧬 Code Definitions (1)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (1)
src/components/ui/Collapsible/contexts/CollapsibleContext.tsx (1)
CollapsibleContext(19-20)
🪛 Biome (1.9.4)
docs/components/layout/Documentation/helpers/CodeBlock.js
[error] 46-46: This JSX property is assigned multiple times.
This attribute is assigned again here.
(lint/suspicious/noDuplicateJsxProps)
🔇 Additional comments (20)
package.json (1)
62-62: Great improvement to the linting process!Changing the stylelint command from
--no-fixto--fixenables automatic fixing of style issues during linting, which will help maintain consistent styling across the codebase and reduce manual fixes.docs/components/layout/Documentation/helpers/CodeBlock.js (3)
1-2: Good addition of React imports and client directiveAdding the "use client" directive and importing useState is necessary for the new interactive functionality.
10-10: Imported Button component for the new toggle functionalityThis is a necessary import for the show more/less button implementation.
34-34: Good addition of state management for code block expansionThe useState hook properly manages the expanded state of the code block.
docs/app/globals.scss (2)
79-86: Good simplification of keyframe syntaxCombining the 0% and 100% keyframes is a clean, efficient approach that maintains the same functionality while reducing redundancy.
90-104: Well-implemented blur effect for collapsed code blocksThe code-block-blur class with the pseudo-element gradient creates a nice visual indication that there's more content below. The implementation uses best practices with pointer-events: none to ensure clicks pass through to underlying elements.
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (2)
27-29: Good addition of RightArrowIcon componentThis SVG icon component is well-structured and provides visual feedback for the collapsible state.
31-74: Improved Collapsible component implementation with compound patternThe refactored Default story showcases a much better implementation using the compound component pattern with Root, Trigger, and Content. This approach:
- Provides better composition and flexibility
- Makes the component more intuitive to use
- Properly handles animation with the transition classes
- Clearly demonstrates the component's usage pattern
The rotating arrow icon with
group-data-[state=open]:rotate-90is a nice touch for visual feedback.src/components/ui/Collapsible/fragments/CollapsibleContent.tsx (3)
4-4: Good addition of CollapsiblePrimitive importThe import of CollapsiblePrimitive is appropriate for the refactoring approach, moving the implementation details to a primitive component.
13-14: Clean context usage simplificationGood refactoring to only extract the
rootClassfrom context rather than unused properties. The newcontentClassvariable creates a cleaner class name derivation approach.
16-16: Appropriate delegation to primitive componentSuccessfully migrated from a custom implementation to using the primitive component, which should provide more consistent behavior. The class composition with clsx is maintained correctly.
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (2)
3-3: Good import of CollapsiblePrimitiveAppropriate addition of the primitive component import to support the refactoring approach.
17-19: Well-implemented trigger refactoringThe refactoring successfully simplifies the component by:
- Only extracting needed context values
- Creating a consistent class naming pattern
- Delegating behavior to the primitive component
This should make the code more maintainable and consistent across the collapsible components.
styles/themes/components/collapsible.scss (4)
1-9: Good styling structure for the trigger componentThe updated class structure uses more semantic naming (
.rad-ui-collapsibleinstead of-root) and properly styles the trigger element with appropriate spacing, colors and dimensions.
11-19: Well-structured content stylingThe content styling provides good visual consistency with appropriate spacing, colors and border-radius.
20-39: Nice implementation of the collapse effect with gradientThe closed state implementation with the gradient overlay creates a nice blur/fade effect. The fixed height of 200px with overflow hidden works well with the gradient to indicate there's more content.
41-44: Good open state handlingUsing
height: auto !importantensures the expanded content displays properly regardless of other style rules that might affect the height.src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx (1)
9-13: Good implementation of CollapsibleRoot with primitive componentThe refactoring correctly:
- Passes only the necessary
rootClassvalue to the context- Uses the primitive component with all relevant props
- Maintains the class name composition pattern
This creates a cleaner interface that delegates behavior to the primitive while maintaining consistency with the rest of the UI components.
src/components/ui/Collapsible/Collapsible.tsx (2)
1-3: No issues found with these imports.
20-22: Exposing subcomponents as static properties is consistent with the compound component pattern.
| <code className={`language-${language} whitespace-pre-wrap`} style={{ wordBreak: 'break-word' }} | ||
| style={{ | ||
| height: expanded ? 'auto' : 180, | ||
| maxHeight: 640, | ||
| overflowY: expanded ? 'scroll' : 'hidden', | ||
|
|
||
| }} | ||
| >{code}</code> |
There was a problem hiding this comment.
Fix duplicate style attribute on code element
There's a duplicate style attribute on the code element which will cause only the second one to be applied.
-<code className={`language-${language} whitespace-pre-wrap`} style={{ wordBreak: 'break-word' }}
- style={{
- height: expanded ? 'auto' : 180,
- maxHeight: 640,
- overflowY: expanded ? 'scroll' : 'hidden',
-
- }}
+<code
+ className={`language-${language} whitespace-pre-wrap`}
+ style={{
+ wordBreak: 'break-word',
+ height: expanded ? 'auto' : 180,
+ maxHeight: 640,
+ overflowY: expanded ? 'scroll' : 'hidden'
+ }}📝 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.
| <code className={`language-${language} whitespace-pre-wrap`} style={{ wordBreak: 'break-word' }} | |
| style={{ | |
| height: expanded ? 'auto' : 180, | |
| maxHeight: 640, | |
| overflowY: expanded ? 'scroll' : 'hidden', | |
| }} | |
| >{code}</code> | |
| <code | |
| className={`language-${language} whitespace-pre-wrap`} | |
| style={{ | |
| wordBreak: 'break-word', | |
| height: expanded ? 'auto' : 180, | |
| maxHeight: 640, | |
| overflowY: expanded ? 'scroll' : 'hidden' | |
| }} | |
| >{code}</code> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: This JSX property is assigned multiple times.
This attribute is assigned again here.
(lint/suspicious/noDuplicateJsxProps)
| const Collapsible = () => { | ||
| console.warn('Direct usage of Tabs is not supported. Please use Tabs.Root, Tabs.List, etc. instead.'); | ||
| return null; |
There was a problem hiding this comment.
Console warning references "Tabs" instead of "Collapsible."
This appears to be an unintentional leftover from another component, which can confuse users.
Here is a suggested fix:
- console.warn('Direct usage of Tabs is not supported. Please use Tabs.Root, Tabs.List, etc. instead.');
+ console.warn('Direct usage of Collapsible is not supported. Please use Collapsible.Root, Collapsible.Trigger, etc. instead.');📝 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 Collapsible = () => { | |
| console.warn('Direct usage of Tabs is not supported. Please use Tabs.Root, Tabs.List, etc. instead.'); | |
| return null; | |
| const Collapsible = () => { | |
| console.warn('Direct usage of Collapsible is not supported. Please use Collapsible.Root, Collapsible.Trigger, etc. instead.'); | |
| return null; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ui/Collapsible/tests/Collapsible.test.tsx (1)
70-79: Good addition of new component test.Adding a dedicated test for the
Collapsible.Contentcomponent is a good practice. It ensures that each component in the composition works correctly in isolation.Consider adding more test cases to verify different states and behaviors of the Content component (e.g., with different children, when it's initially collapsed, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/Collapsible/tests/Collapsible.test.tsx(2 hunks)
🔇 Additional comments (3)
src/components/ui/Collapsible/tests/Collapsible.test.tsx (3)
8-13: Good refactoring to a more composable pattern!The change to use the compositional pattern with
Collapsible.Root,Collapsible.Trigger, andCollapsible.Contentis a great improvement. This approach follows modern React component design patterns (similar to Radix UI or Headless UI) and provides more flexibility for consumers of the component.
21-26: Props correctly moved to appropriate component.The
disabledprop has been correctly moved to theCollapsible.Rootcomponent, which is consistent with the new component structure. This maintains the same functionality while adapting to the new pattern.
59-65: Consistent naming update.The test suite name has been appropriately updated to match the new compositional pattern, and the test implementation follows the new structure correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/ui/Collapsible/tests/Collapsible.test.tsx (1)
5-141: Consider adding tests for transitionDuration propWhile the tests cover most functionality, there's no test for the newly added transitionDuration prop on CollapsibleRoot. Consider adding a test that verifies this prop is properly passed to the underlying component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx(1 hunks)src/components/ui/Collapsible/tests/Collapsible.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx (3)
5-5: Good addition of primitive componentUsing the CollapsiblePrimitive from core/primitives is a good architectural choice that promotes better separation of concerns.
9-18: Well-defined props interfaceThe CollapsibleRootProps type is now properly defined with all necessary properties. This addresses the previous TypeScript error and provides good documentation for component usage.
20-24: Cleaner implementation with proper prop forwardingThe refactored implementation properly forwards all relevant props to the CollapsiblePrimitive.Root component while maintaining the context provider for consistency. This is a good improvement that simplifies the component structure.
src/components/ui/Collapsible/tests/Collapsible.test.tsx (5)
6-23: Good mock implementation for useControllableStateThis mock properly simulates both controlled and uncontrolled behavior, ensuring that tests accurately reflect component functionality. The mock correctly updates state and calls callbacks when necessary.
27-37: Updated test structure to match new component APIThe test now correctly uses the new component structure with Collapsible.Root, Collapsible.Trigger, and Collapsible.Content, ensuring proper validation of the new implementation.
39-59: Improved test for disabled stateThe test now properly verifies that:
- The onOpenChange callback is not called when disabled
- Content remains visible despite clicking the trigger
This is a more thorough validation of the disabled behavior.
61-93: Good controlled component test implementationThis test thoroughly verifies the controlled component behavior by:
- Testing initial closed state
- Verifying callback is called with correct value
- Testing content visibility after parent-controlled state change
This addresses the previous review comment about toggle verification.
96-141: Thorough subcomponent testingThe tests for Collapsible.Trigger and Collapsible.Content are well-structured and verify both content rendering and ARIA attributes, which is great for accessibility.
Summary by CodeRabbit
New Features
.code-block-blurclass for enhanced visual effects on code blocks.Refactor
Collapsiblecomponent for a more modular structure, utilizing new fragment components for better organization.Style