improve Collapsible component UI + APIs #528 #663
Conversation
WalkthroughThis pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Nitpick comments (14)
src/components/ui/Collapsible/Collapsible.tsx (3)
1-7: Remove unnecessary trailing comma in import statement.
ESLint flags a trailing comma after "useState". It's generally good practice to remove trailing commas in import statements to maintain a clean style, unless your team's style guide mandates them.-} from "react"; +} from "react"🧰 Tools
🪛 eslint
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
57-57: Remove or align trailing commas in object literals.
ESLint also flags a trailing comma in style objects which might be disallowed depending on your style configuration. Consider removing it to align with your code style guidelines.-justifyContent: "space-between", +justifyContent: "space-between"🧰 Tools
🪛 eslint
[error] 57-57: Unexpected trailing comma.
(comma-dangle)
72-75: Break up the multiline ternary for readability.
ESLint suggests new lines around the ternary expression. This improves readability and makes the conditional rendering clearer.-{disabled ? ( - items.map((item) => <CollapsibleItem>{item.content}</CollapsibleItem>) -) : ( - ... -)} +{ + disabled + ? items.map((item) => <CollapsibleItem>{item.content}</CollapsibleItem>) + : ( + ... + ) +}🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🪛 eslint
[error] 72-72: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 72-75: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx (3)
11-11: Fix spacing around curly braces and operators.
ESLint indicates minor spacing issues around your destructured props. Appropriate spacing improves readability.-const CollapsibleContent: React.FC<CollapsibleContentProps> = ({children,className='',state}:CollapsibleContentProps) => { +const CollapsibleContent: React.FC<CollapsibleContentProps> = ({ children, className = '', state }: CollapsibleContentProps) => {🧰 Tools
🪛 eslint
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
21-21: Remove trailing comma in the style object.
ESLint warns about the trailing comma in the style object. Removing it is often recommended unless otherwise enforced by a code style.- height: (state) ? "auto" : "0", + height: state ? "auto" : "0"🧰 Tools
🪛 eslint
[error] 21-21: Unexpected trailing comma.
(comma-dangle)
29-29: Add a newline at the end of the file.
Some linters or style guides require this. It also prevents potential issues with Unix-based tooling.🧰 Tools
🪛 eslint
[error] 29-29: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx (3)
6-9: Remove extra blank lines for cleaner code.
ESLint flags multiple consecutive blank lines. Consolidating them helps maintain consistency.-🧰 Tools
🪛 eslint
[error] 6-9: More than 1 blank line not allowed.
(no-multiple-empty-lines)
18-20: Fix spacing around curly braces.
As per ESLint's style rules, add proper spacing around the object destructuring.-const CollapsibleRoot = ({children, customRootClass, open, onOpenChange}: CollapsibleRootProps) => { +const CollapsibleRoot = ({ children, customRootClass, open, onOpenChange }: CollapsibleRootProps) => {🧰 Tools
🪛 eslint
[error] 18-18: A space is required after '{'.
(object-curly-spacing)
[error] 18-18: A space is required before '}'.
(object-curly-spacing)
[error] 18-20: Block must not be padded by blank lines.
(padded-blocks)
34-34: Add newline at end of file.
Including a newline at the end of file typically aligns with most style guides and avoids warnings in version control systems.🧰 Tools
🪛 eslint
[error] 34-34: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (2)
11-43: Consider making icon size configurableThe icons use a hardcoded
size-6class. Consider making the size configurable through props for better flexibility in different contexts.- className="size-6" + className={`size-${size || 6}`}
46-61: Improve code formatting and simplify conditional rendering
- Add spaces in object destructuring
- Consider simplifying the conditional rendering
-const CollapsibleTrigger = ({children}: CollapsibleTriggerProps) => { - const {onOpenChange, open,rootClass} = useContext(CollapsibleContext) +const CollapsibleTrigger = ({ children }: CollapsibleTriggerProps) => { + const { onOpenChange, open, rootClass } = useContext(CollapsibleContext) - {children ? ( - children - ) : ( - <ButtonPrimitive onClick={toggleCollapse} className={clsx(`${rootClass}-button`)}> - {open ? <CollapseIcon /> : <ExpandIcon />} - </ButtonPrimitive> - )} + {children || ( + <ButtonPrimitive onClick={toggleCollapse} className={clsx(`${rootClass}-button`)}> + {open ? <CollapseIcon /> : <ExpandIcon />} + </ButtonPrimitive> + )}🧰 Tools
🪛 eslint
[error] 46-46: A space is required after '{'.
(object-curly-spacing)
[error] 46-46: A space is required before '}'.
(object-curly-spacing)
[error] 47-47: A space is required after '{'.
(object-curly-spacing)
[error] 47-47: A space is required before '}'.
(object-curly-spacing)
[error] 52-52: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 52-58: Unnecessary use of conditional expression for default assignment.
(no-unneeded-ternary)
[error] 52-54: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (2)
7-19: Consider enhancing the Items data structureThe current structure could be improved to better represent collapsible items:
- Add TypeScript interface
- Include optional properties for item customization
+interface CollapsibleItem { + id?: string; + title?: string; + content: string; + disabled?: boolean; +} -const Items = [ +const Items: CollapsibleItem[] = [ { + id: '1', + title: 'Plato on Politics', content: "One of the penalties...", }, // ... other items ];🧰 Tools
🪛 eslint
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
[error] 14-14: Unexpected trailing comma.
(comma-dangle)
[error] 17-17: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
31-33: Clarify the purpose of the disabled propThe
disabledprop is used in multiple stories without demonstrating the difference between enabled and disabled states. Consider:
- Adding a story that shows both states
- Adding comments explaining the expected behavior
Also applies to: 42-43
src/components/ui/Collapsible/contexts/CollapsibleContext.tsx (1)
3-3: Add newline at end of fileAdd a newline at the end of the file to comply with eslint rules.
export const CollapsibleContext = createContext({}); +🧰 Tools
🪛 eslint
[error] 3-3: Newline required at end of file but not found.
(eol-last)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/contexts/CollapsibleContext.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleContent.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleItem.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx(1 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(1 hunks)styles/themes/components/collapsible.scss(1 hunks)styles/themes/default.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- styles/themes/components/collapsible.scss
🧰 Additional context used
🪛 eslint
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx
[error] 6-9: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 18-18: A space is required after '{'.
(object-curly-spacing)
[error] 18-18: A space is required before '}'.
(object-curly-spacing)
[error] 18-20: Block must not be padded by blank lines.
(padded-blocks)
[error] 34-34: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx
[error] 6-7: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 45-46: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 46-46: A space is required after '{'.
(object-curly-spacing)
[error] 46-46: A space is required before '}'.
(object-curly-spacing)
[error] 47-47: A space is required after '{'.
(object-curly-spacing)
[error] 47-47: A space is required before '}'.
(object-curly-spacing)
[error] 52-52: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 52-58: Unnecessary use of conditional expression for default assignment.
(no-unneeded-ternary)
[error] 52-54: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 63-63: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleItem.tsx
[error] 12-12: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
[error] 14-14: Unexpected trailing comma.
(comma-dangle)
[error] 17-17: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/Collapsible.tsx
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
[error] 57-57: Unexpected trailing comma.
(comma-dangle)
[error] 72-72: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 72-75: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 13-13: A space is required after '{'.
(object-curly-spacing)
[error] 13-13: A space is required before '}'.
(object-curly-spacing)
[error] 21-21: Unexpected trailing comma.
(comma-dangle)
[error] 29-29: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/contexts/CollapsibleContext.tsx
[error] 3-3: Newline required at end of file but not found.
(eol-last)
🪛 Biome (1.9.4)
src/components/ui/Collapsible/Collapsible.tsx
[error] 74-74: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 84-88: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (3)
src/components/ui/Collapsible/fragments/CollapsibleItem.tsx (1)
1-21: Looks good overall.
The code is straightforward and aligns well with React best practices. The static analysis about trailing commas seems to be a false positive here. No trailing commas actually present.
🧰 Tools
🪛 eslint
[error] 12-12: Unexpected trailing comma.
(comma-dangle)
styles/themes/default.scss (1)
26-26: LGTM!
The collapsible component styles import follows the established pattern of component imports.
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (1)
1-9: LGTM! Clean imports and type definition.
The imports are well-organized and the type definition is clear and focused.
🧰 Tools
🪛 eslint
[error] 6-7: More than 1 blank line not allowed.
(no-multiple-empty-lines)
| <> | ||
| {item != defaultOpen && ( | ||
| <CollapsibleItem>{item.content}</CollapsibleItem> | ||
| )} | ||
| </> |
There was a problem hiding this comment.
Add a unique key for each item in the map.
Similarly, the items rendered here need a “key” prop to help React identify them.
{items.map((item) => (
<>
{item != defaultOpen && (
- <CollapsibleItem>{item.content}</CollapsibleItem>
+ <CollapsibleItem key={item.content}>{item.content}</CollapsibleItem>
)}
</>
))}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 84-88: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
| {/* Conditonal Loop */} | ||
| {disabled ? ( | ||
| // loops through all the items with no conditions | ||
| items.map((item) => <CollapsibleItem>{item.content}</CollapsibleItem>) |
There was a problem hiding this comment.
Add a unique key for each item in the map.
React requires a “key” prop when rendering an array of elements to help it track element identity and improve rendering performance.
-items.map((item) => <CollapsibleItem>{item.content}</CollapsibleItem>)
+items.map((item, index) => (
+ <CollapsibleItem key={index}>{item.content}</CollapsibleItem>
+))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
| <Collapsible | ||
| open={open} | ||
| onOpenChange={setOpen} | ||
| title="Quote" | ||
| items={Items} | ||
| defaultOpen={Items[0]} | ||
| trigger={ | ||
| <Button onClick={toggleHidden} style={{ margin: "0" }}> | ||
| {open ? "CLOSE" : "OPEN"} | ||
| </Button> | ||
| } | ||
| /> |
There was a problem hiding this comment.
Potential conflict between controlled and uncontrolled states
The component receives both open (controlled) and defaultOpen (uncontrolled) props. This could lead to unexpected behavior. Consider:
- Using either controlled or uncontrolled state, not both
- Adding a comment explaining the precedence
<Collapsible
open={open}
onOpenChange={setOpen}
title="Quote"
items={Items}
- defaultOpen={Items[0]}
trigger={
<Button onClick={toggleHidden} style={{ margin: "0" }}>
{open ? "CLOSE" : "OPEN"}
</Button>
}
/>📝 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.
| <Collapsible | |
| open={open} | |
| onOpenChange={setOpen} | |
| title="Quote" | |
| items={Items} | |
| defaultOpen={Items[0]} | |
| trigger={ | |
| <Button onClick={toggleHidden} style={{ margin: "0" }}> | |
| {open ? "CLOSE" : "OPEN"} | |
| </Button> | |
| } | |
| /> | |
| <Collapsible | |
| open={open} | |
| onOpenChange={setOpen} | |
| title="Quote" | |
| items={Items} | |
| trigger={ | |
| <Button onClick={toggleHidden} style={{ margin: "0" }}> | |
| {open ? "CLOSE" : "OPEN"} | |
| </Button> | |
| } | |
| /> |
| import { createContext } from "react"; | ||
|
|
||
| export const CollapsibleContext = createContext({}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript interface and meaningful default values
The context is created without type information or proper default values. This makes it harder for consumers to understand what values are available and TypeScript won't be able to provide proper type checking.
Consider adding proper typing:
-import { createContext } from "react";
+import { createContext, Dispatch, SetStateAction } from "react";
+interface CollapsibleContextType {
+ rootClass?: string;
+ isOpen?: boolean;
+ setIsOpen?: Dispatch<SetStateAction<boolean>>;
+ disabled?: boolean;
+ defaultOpen?: boolean;
+}
+
+const defaultContext: CollapsibleContextType = {
+ rootClass: '',
+ isOpen: false,
+ setIsOpen: () => {},
+ disabled: false,
+ defaultOpen: false,
+};
-export const CollapsibleContext = createContext({});
+export const CollapsibleContext = createContext<CollapsibleContextType>(defaultContext);📝 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.
| import { createContext } from "react"; | |
| export const CollapsibleContext = createContext({}); | |
| import { createContext, Dispatch, SetStateAction } from "react"; | |
| interface CollapsibleContextType { | |
| rootClass?: string; | |
| isOpen?: boolean; | |
| setIsOpen?: Dispatch<SetStateAction<boolean>>; | |
| disabled?: boolean; | |
| defaultOpen?: boolean; | |
| } | |
| const defaultContext: CollapsibleContextType = { | |
| rootClass: '', | |
| isOpen: false, | |
| setIsOpen: () => {}, | |
| disabled: false, | |
| defaultOpen: false, | |
| }; | |
| export const CollapsibleContext = createContext<CollapsibleContextType>(defaultContext); |
🧰 Tools
🪛 eslint
[error] 3-3: Newline required at end of file but not found.
(eol-last)
Screen.Recording.2024-12-21.120209.mp4A video example |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (1)
56-67:⚠️ Potential issuePotential conflict between controlled and uncontrolled states
The component receives both controlled (
open,onOpenChange) and uncontrolled (defaultOpen) props simultaneously, which could lead to unexpected behavior.Apply this diff to resolve the conflict:
<Collapsible open={open} onOpenChange={setOpen} title="Quote" items={Items} - defaultOpen={Items[0]} trigger={ <Button style={{ margin: "0" }}> {open ? "CLOSE" : "OPEN"} </Button> } />
🧹 Nitpick comments (15)
src/components/ui/Collapsible/contexts/CollapsibleContext.tsx (3)
3-9: Consider making some context properties requiredThe type definition makes all properties optional with
?, but some properties likeopenandonOpenChangeseem fundamental to the component's functionality. Consider making these required to ensure proper usage.type CollapsibleContextType = { rootClass?: string; - open?: boolean; - onOpenChange?: Dispatch<SetStateAction<boolean>>; + open: boolean; + onOpenChange: Dispatch<SetStateAction<boolean>>; disabled?: boolean; defaultOpen?: boolean; };
11-17: Add JSDoc documentation for the contextThe context and its default values would benefit from documentation explaining their purpose and usage.
+/** + * Context for managing collapsible component state + * @property rootClass - Base class for styling + * @property open - Current open state + * @property onOpenChange - Callback to update open state + * @property disabled - Whether the collapsible is disabled + * @property defaultOpen - Initial open state + */ const defaultContext: CollapsibleContextType = {🧰 Tools
🪛 eslint
[error] 16-16: Unexpected trailing comma.
(comma-dangle)
19-20: Fix missing newline at end of fileAdd a newline at the end of the file to follow standard conventions.
export const CollapsibleContext = - createContext<CollapsibleContextType>(defaultContext); + createContext<CollapsibleContextType>(defaultContext); +🧰 Tools
🪛 eslint
[error] 20-20: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleHeader.tsx (1)
11-16: Improve code formatting and readabilityThe component implementation has several formatting issues and could be more readable:
- Missing spaces in destructuring
- Long line in JSX should be split for better readability
- Consider adding prop validation for title length
-const CollapsibleHeader = ({children,className="",title=""}: CollapsibleHeaderProps) => { +const CollapsibleHeader = ({ children, className = "", title = "" }: CollapsibleHeaderProps) => { const { rootClass } = useContext(CollapsibleContext) return ( - <div className={clsx(`${rootClass}-header`,className)}>{title && <p>{title}</p>}{children}</div> + <div className={clsx(`${rootClass}-header`, className)}> + {title && <p>{title}</p>} + {children} + </div> ) }🧰 Tools
🪛 eslint
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Collapsible/fragments/CollapsibleItem.tsx (2)
5-9: Remove extra whitespace in type definitionThere's unnecessary whitespace in the type definition.
type CollapsibleItemProps = { children: React.ReactNode; className?: string; - };
11-21: Simplify component definition and fix formattingThe component definition can be simplified and formatting improved:
- Remove redundant type annotation and prop type
- Remove extra whitespace and trailing comma
- Fix space before closing bracket in JSX
-const CollapsibleItem: React.FC<CollapsibleItemProps> = ({ - children, - className = "", - -}: CollapsibleItemProps) => { +const CollapsibleItem = ({ + children, + className = "" +}: CollapsibleItemProps) => { const { rootClass } = useContext(CollapsibleContext); return ( - <div className={clsx(`${rootClass}-item`, className)} >{children}</div> + <div className={clsx(`${rootClass}-item`, className)}>{children}</div> ); };🧰 Tools
🪛 eslint
[error] 13-13: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx (2)
5-9: Consider enhancing prop types documentationThe
CollapsibleContentPropsinterface could benefit from JSDoc comments explaining:
- The purpose of the
stateprop- Whether
classNamefollows any specific patterntype CollapsibleContentProps = { + /** The content to be rendered inside the collapsible section */ children: React.ReactNode; + /** Additional CSS classes to be applied to the content wrapper */ className?: string; + /** Controls the visibility of the content. true = expanded, false = collapsed */ state: boolean }
11-24: Fix formatting issues and improve code structureThe component implementation has several formatting inconsistencies.
-const CollapsibleContent: React.FC<CollapsibleContentProps> = ({children,className='',state}:CollapsibleContentProps) => { +const CollapsibleContent: React.FC<CollapsibleContentProps> = ({ + children, + className = '', + state +}: CollapsibleContentProps) => { + const { rootClass } = useContext(CollapsibleContext); - const {rootClass} = useContext(CollapsibleContext) - - return ( - <div - className={clsx(`${rootClass}-content`, className)} - aria-hidden={!state} - data-state={state? "expanded" : "collapsed"} - > - {state && children} - </div> - ); + return ( + <div + className={clsx(`${rootClass}-content`, className)} + aria-hidden={!state} + data-state={state ? "expanded" : "collapsed"} + > + {state && children} + </div> + ); }🧰 Tools
🪛 eslint
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 13-13: A space is required after '{'.
(object-curly-spacing)
[error] 13-13: A space is required before '}'.
(object-curly-spacing)
[error] 19-19: Operator '?' must be spaced.
(space-infix-ops)
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx (3)
6-9: Remove excessive empty linesMultiple consecutive empty lines should be removed to maintain code cleanliness.
🧰 Tools
🪛 eslint
[error] 6-9: More than 1 blank line not allowed.
(no-multiple-empty-lines)
11-17: Add JSDoc documentation for propsThe
CollapsibleRootPropstype would benefit from detailed documentation.+/** + * Props for the CollapsibleRoot component + */ export type CollapsibleRootProps = { + /** The content to be rendered inside the collapsible root */ children: React.ReactNode; + /** Optional custom class name for the root element */ customRootClass?: string; + /** Controls the expanded/collapsed state */ open: boolean; + /** Callback function to handle state changes */ onOpenChange: Dispatch<SetStateAction<boolean>>; + /** Additional CSS classes to be applied */ className?: string };
19-33: Improve component structure and formattingThe component's JSX structure could be more readable with proper indentation and line breaks.
-const CollapsibleRoot = ({children,className="", customRootClass, open, onOpenChange}: CollapsibleRootProps) => { +const CollapsibleRoot = ({ + children, + className = "", + customRootClass, + open, + onOpenChange +}: CollapsibleRootProps) => { + const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); - const rootClass = customClassSwitcher(customRootClass,COMPONENT_NAME) - - return ( - <CollapsibleContext.Provider - value={{ - rootClass, - open, - onOpenChange - }} - ><div className={clsx(`${rootClass}-root`,className)}> - {children}</div></CollapsibleContext.Provider> - ) + return ( + <CollapsibleContext.Provider + value={{ + rootClass, + open, + onOpenChange + }} + > + <div className={clsx(`${rootClass}-root`, className)}> + {children} + </div> + </CollapsibleContext.Provider> + ); }🧰 Tools
🪛 eslint
[error] 19-19: A space is required after '{'.
(object-curly-spacing)
[error] 19-19: Operator '=' must be spaced.
(space-infix-ops)
[error] 19-19: A space is required before '}'.
(object-curly-spacing)
[error] 19-21: Block must not be padded by blank lines.
(padded-blocks)
styles/themes/components/collapsible.scss (2)
1-4: Consider making root width configurableThe hard-coded 70% width might limit component reusability in different contexts.
.rad-ui-collapsible-root{ - width: 70%; + width: var(--rad-ui-collapsible-width, 70%); }
15-21: Implement collapsed and expanded statesThe TODO comments should be addressed to ensure proper animation and visibility states.
Would you like me to provide implementations for the collapsed and expanded states with smooth transitions?
src/components/ui/Collapsible/tests/Collapsible.test.tsx (2)
55-60: Fix formatting issues.Remove extra blank lines to comply with the project's formatting rules.
}); - -}); - +});🧰 Tools
🪛 eslint
[error] 55-58: Block must not be padded by blank lines.
(padded-blocks)
[error] 57-58: More than 1 blank line not allowed.
(no-multiple-empty-lines)
111-158: Enhance test coverage for fragment components.While the basic functionality is covered, consider adding these test cases for better coverage:
CollapsibleTrigger:
- Test click handler functionality
- Test keyboard interactions (Enter/Space)
CollapsibleContent:
- Test transition/animation states if applicable
- Test aria attributes for accessibility
All components:
- Test prop validation and error cases
- Test edge cases (empty content, missing required props)
Here's an example for CollapsibleTrigger:
it("handles click events", () => { const onClickMock = jest.fn(); const { getByRole } = render( <CollapsibleTrigger onClick={onClickMock}> <button>Trigger</button> </CollapsibleTrigger> ); fireEvent.click(getByRole('button')); expect(onClickMock).toHaveBeenCalled(); }); it("handles keyboard events", () => { const onClickMock = jest.fn(); const { getByRole } = render( <CollapsibleTrigger onClick={onClickMock}> <button>Trigger</button> </CollapsibleTrigger> ); const button = getByRole('button'); fireEvent.keyDown(button, { key: 'Enter' }); fireEvent.keyDown(button, { key: ' ' }); expect(onClickMock).toHaveBeenCalledTimes(2); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/contexts/CollapsibleContext.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleContent.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleHeader.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleItem.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx(1 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(1 hunks)src/components/ui/Collapsible/tests/Collapsible.test.tsx(1 hunks)styles/themes/components/collapsible.scss(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Collapsible/contexts/CollapsibleContext.tsx
[error] 16-16: Unexpected trailing comma.
(comma-dangle)
[error] 20-20: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleHeader.tsx
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 18-18: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/tests/Collapsible.test.tsx
[error] 55-58: Block must not be padded by blank lines.
(padded-blocks)
[error] 57-58: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 60-61: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 106-109: Block must not be padded by blank lines.
(padded-blocks)
[error] 108-109: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx
[error] 6-7: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 47-49: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 49-49: A space is required after '{'.
(object-curly-spacing)
[error] 49-49: Operator '=' must be spaced.
(space-infix-ops)
[error] 49-49: A space is required before '}'.
(object-curly-spacing)
[error] 50-50: A space is required after '{'.
(object-curly-spacing)
[error] 50-50: A space is required before '}'.
(object-curly-spacing)
[error] 53-54: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 56-56: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 56-56: Operator '?' must be spaced.
(space-infix-ops)
[error] 56-56: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 56-56: Operator ':' must be spaced.
(space-infix-ops)
[error] 60-61: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 65-65: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx
[error] 6-9: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 19-19: A space is required after '{'.
(object-curly-spacing)
[error] 19-19: Operator '=' must be spaced.
(space-infix-ops)
[error] 19-19: A space is required before '}'.
(object-curly-spacing)
[error] 19-21: Block must not be padded by blank lines.
(padded-blocks)
[error] 35-35: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
[error] 14-14: Unexpected trailing comma.
(comma-dangle)
[error] 17-17: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/Collapsible.tsx
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
[error] 38-39: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 66-66: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 66-71: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 13-13: A space is required after '{'.
(object-curly-spacing)
[error] 13-13: A space is required before '}'.
(object-curly-spacing)
[error] 19-19: Operator '?' must be spaced.
(space-infix-ops)
[error] 26-26: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/fragments/CollapsibleItem.tsx
[error] 13-13: Unexpected trailing comma.
(comma-dangle)
🪛 Biome (1.9.4)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/ui/Collapsible/Collapsible.tsx
[error] 80-84: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (1)
src/components/ui/Collapsible/tests/Collapsible.test.tsx (1)
1-9: LGTM! Well-organized imports.
The imports are logically grouped and include all necessary testing utilities and components.
| <Button style={{ margin: "0" }}> | ||
| {open ? "CLOSE" : "OPEN"} | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant onClick handler
The onClick handler is already provided by the CollapsibleTrigger component, making the Button's onClick prop unnecessary.
Apply this diff:
- <Button onClick={toggleHidden} style={{ margin: "0" }}>
+ <Button style={{ margin: "0" }}>
{open ? "CLOSE" : "OPEN"}
</Button>📝 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.
| <Button style={{ margin: "0" }}> | |
| {open ? "CLOSE" : "OPEN"} | |
| </Button> | |
| <Button style={{ margin: "0" }}> | |
| {open ? "CLOSE" : "OPEN"} | |
| </Button> |
| {disabled ? ( | ||
| // loops through all the items with no conditions | ||
| items.map((item, index) => ( | ||
| <CollapsibleItem key={index}>{item.content}</CollapsibleItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array indices as keys
Using array indices as keys can lead to rendering issues if the array items are reordered or modified.
Consider adding a unique identifier to your items type:
- items: { content: string | ReactNode }[];
+ items: { id: string; content: string | ReactNode }[];Then use it as the key:
- <CollapsibleItem key={index}>{item.content}</CollapsibleItem>
+ <CollapsibleItem key={item.id}>{item.content}</CollapsibleItem>Also applies to: 82-82
| it("disables collapsible when disabled prop is true", () => { | ||
| const { getByText } = render( | ||
| <Collapsible disabled={true} items={[{ content: "Item 1" }]} /> | ||
| ); | ||
| expect(getByText("Item 1")).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Improve disabled state test coverage.
The current test only verifies rendering but doesn't validate that the collapsible functionality is actually disabled. Consider adding interaction tests to verify that clicking the trigger doesn't toggle the content when disabled.
Here's a suggested improvement:
it("disables collapsible when disabled prop is true", () => {
- const { getByText } = render(
+ const { getByText, queryByText } = render(
- <Collapsible disabled={true} items={[{ content: "Item 1" }]} />
+ <Collapsible
+ disabled={true}
+ trigger={<button>Toggle</button>}
+ items={[{ content: "Item 1" }]}
+ />
);
- expect(getByText("Item 1")).toBeInTheDocument();
+ const trigger = getByText("Toggle");
+ const initialState = queryByText("Item 1");
+ fireEvent.click(trigger);
+ expect(queryByText("Item 1")).toBe(initialState); // Content state should not change📝 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.
| it("disables collapsible when disabled prop is true", () => { | |
| const { getByText } = render( | |
| <Collapsible disabled={true} items={[{ content: "Item 1" }]} /> | |
| ); | |
| expect(getByText("Item 1")).toBeInTheDocument(); | |
| }); | |
| it("disables collapsible when disabled prop is true", () => { | |
| const { getByText, queryByText } = render( | |
| <Collapsible | |
| disabled={true} | |
| trigger={<button>Toggle</button>} | |
| items={[{ content: "Item 1" }]} | |
| /> | |
| ); | |
| const trigger = getByText("Toggle"); | |
| const initialState = queryByText("Item 1"); | |
| fireEvent.click(trigger); | |
| expect(queryByText("Item 1")).toBe(initialState); // Content state should not change | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (1)
56-68:⚠️ Potential issuePotential conflict between controlled and uncontrolled states
The component receives both
open(controlled) anddefaultOpen(uncontrolled) props which could lead to unexpected behavior.<Collapsible open={open} onOpenChange={setOpen} title="Quote" items={Items} - defaultOpen={Items[0]} disabled={true} trigger={ <Button style={{ margin: "0" }}> {open ? "CLOSE" : "OPEN"} </Button> } />src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (1)
49-51: 🛠️ Refactor suggestionImprove error handling and type safety
The context values are destructured without checking if they exist, and the toggle logic could be simplified.
-const CollapsibleTrigger = ({children, asChild=false}: CollapsibleTriggerProps) => { - const {rootClass,open,onOpenChange,disabled} = useContext(CollapsibleContext) - const toggleCollapse = () => onOpenChange && !disabled && onOpenChange((p) => (!p)) +const CollapsibleTrigger = ({ children, asChild = false }: CollapsibleTriggerProps) => { + const { rootClass, open, onOpenChange, disabled } = useContext(CollapsibleContext); + + if (!rootClass || typeof open !== 'boolean' || !onOpenChange) { + throw new Error('CollapsibleTrigger must be used within a CollapsibleRoot'); + } + + const toggleCollapse = () => { + if (!disabled) { + onOpenChange(prev => !prev); + } + };🧰 Tools
🪛 eslint
[error] 49-49: A space is required after '{'.
(object-curly-spacing)
[error] 49-49: Operator '=' must be spaced.
(space-infix-ops)
[error] 49-49: A space is required before '}'.
(object-curly-spacing)
[error] 50-50: A space is required after '{'.
(object-curly-spacing)
[error] 50-50: A space is required before '}'.
(object-curly-spacing)
🧹 Nitpick comments (7)
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx (1)
11-18: Consider adding prop validation for disabled stateThe
disabledprop should have validation to ensure it's a boolean value when provided.export type CollapsibleRootProps = { children: React.ReactNode; customRootClass?: string; open: boolean; onOpenChange: Dispatch<SetStateAction<boolean>>; className?: string - disabled?: boolean + disabled?: boolean; + /** @default false */ };src/components/ui/Collapsible/stories/Collapsible.stories.tsx (1)
31-33: Improve stories to demonstrate disabled functionalityThe stories should demonstrate both enabled and disabled states for better documentation.
Consider adding a boolean state to toggle the disabled prop:
export const Default = () => { + const [isDisabled, setIsDisabled] = useState(false); return ( <section> <SandboxEditor className=""> + <Button onClick={() => setIsDisabled(!isDisabled)}> + Toggle Disabled + </Button> - <Collapsible items={Items} disabled></Collapsible> + <Collapsible items={Items} disabled={isDisabled}></Collapsible> </SandboxEditor> </section> ); };Also applies to: 41-43
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (1)
55-70: Improve accessibility implementationWhile basic accessibility is implemented, it could be enhanced with additional ARIA attributes and disabled state handling.
<div className={clsx(`${rootClass}-trigger`)} role="button" tabIndex={0} + aria-disabled={disabled} + aria-controls={`${rootClass}-content`} onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); toggleCollapse(); } }} aria-expanded={open} - onClick={ - - toggleCollapse - } + onClick={toggleCollapse} >src/components/ui/Collapsible/Collapsible.tsx (2)
24-32: Consider makingitemsprop optional for better backward compatibilityThe
itemsprop is currently required, which might break existing implementations. Consider making it optional with an empty array as default.export type CollapsibleProps = { open?: boolean; title?: string; trigger?: ReactNode; - items: { content: string | ReactNode }[]; + items?: { content: string | ReactNode }[]; disabled?: boolean; defaultOpen?: { content: string | ReactNode }; onOpenChange?: Dispatch<SetStateAction<boolean>>; } & PropsWithChildren;
59-62: Improve trigger implementation and documentationThe
asChildprop's purpose isn't clear, and the trigger rendering could be more concise.- <CollapsibleTrigger asChild > - {props.trigger && props.trigger} - - </CollapsibleTrigger> + <CollapsibleTrigger asChild> + {props.trigger} + </CollapsibleTrigger>Consider adding JSDoc comments to document the purpose of the
asChildprop.src/components/ui/Collapsible/tests/Collapsible.test.tsx (2)
10-65: Enhance test coverage with additional scenariosWhile the basic functionality is well covered, consider adding tests for:
- Error handling when invalid props are provided
- Controlled vs uncontrolled component behavior
- Edge cases with empty or invalid items
- Accessibility features (ARIA attributes)
Example test cases to add:
it("handles empty items array gracefully", () => { const { container } = render(<Collapsible items={[]} />); expect(container).toBeInTheDocument(); }); it("works as controlled component with external state", () => { const onOpenChange = jest.fn(); render( <Collapsible open={true} onOpenChange={onOpenChange} items={[{ content: "Item 1" }]} /> ); expect(onOpenChange).not.toHaveBeenCalled(); });🧰 Tools
🪛 eslint
[error] 62-65: Block must not be padded by blank lines.
(padded-blocks)
[error] 64-65: More than 1 blank line not allowed.
(no-multiple-empty-lines)
67-69: Fix code style: Remove extra empty linesRemove the multiple empty lines between test suites to maintain consistent spacing.
}); - - describe("CollapsibleHeader Component", () => {🧰 Tools
🪛 eslint
[error] 67-70: More than 1 blank line not allowed.
(no-multiple-empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx(1 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(1 hunks)src/components/ui/Collapsible/tests/Collapsible.test.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Collapsible/fragments/CollapsibleRoot.tsx
[error] 6-9: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 20-20: A space is required after '{'.
(object-curly-spacing)
[error] 20-20: Operator '=' must be spaced.
(space-infix-ops)
[error] 20-20: A space is required before '}'.
(object-curly-spacing)
[error] 20-22: Block must not be padded by blank lines.
(padded-blocks)
[error] 37-37: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/tests/Collapsible.test.tsx
[error] 62-65: Block must not be padded by blank lines.
(padded-blocks)
[error] 64-65: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 67-70: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx
[error] 6-7: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 47-49: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 49-49: A space is required after '{'.
(object-curly-spacing)
[error] 49-49: Operator '=' must be spaced.
(space-infix-ops)
[error] 49-49: A space is required before '}'.
(object-curly-spacing)
[error] 50-50: A space is required after '{'.
(object-curly-spacing)
[error] 50-50: A space is required before '}'.
(object-curly-spacing)
[error] 53-54: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 71-71: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 71-73: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 82-82: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/Collapsible.tsx
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
[error] 38-39: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 67-67: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 67-72: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
[error] 14-14: Unexpected trailing comma.
(comma-dangle)
[error] 17-17: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Unexpected trailing comma.
(comma-dangle)
🔇 Additional comments (1)
src/components/ui/Collapsible/Collapsible.tsx (1)
81-81:
Fix object comparison logic
The current comparison item !== defaultOpen will not work as expected for object comparison. Consider adding an id field to items and comparing by id instead.
- item !== defaultOpen && (
+ item.id !== defaultOpen?.id && (Likely invalid or redundant comment.
| const CollapsibleRoot = ({children,className="",disabled, customRootClass, open, onOpenChange}: CollapsibleRootProps) => { | ||
|
|
||
| const rootClass = customClassSwitcher(customRootClass,COMPONENT_NAME) | ||
|
|
||
| return ( | ||
| <CollapsibleContext.Provider | ||
| value={{ | ||
| rootClass, | ||
| open, | ||
| onOpenChange, | ||
| disabled | ||
| }} | ||
| ><div className={clsx(`${rootClass}-root`,className)}> | ||
| {children}</div></CollapsibleContext.Provider> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve component implementation
- Add spaces in object destructuring
- Add type checking for required context values
- Fix formatting
-const CollapsibleRoot = ({children,className="",disabled, customRootClass, open, onOpenChange}: CollapsibleRootProps) => {
+const CollapsibleRoot = ({
+ children,
+ className = "",
+ disabled = false,
+ customRootClass,
+ open,
+ onOpenChange
+}: CollapsibleRootProps) => {
+ if (typeof open !== 'boolean') {
+ throw new Error('open prop must be a boolean');
+ }
+
const rootClass = customClassSwitcher(customRootClass,COMPONENT_NAME)
return (
<CollapsibleContext.Provider
value={{
rootClass,
open,
onOpenChange,
disabled
}}
- ><div className={clsx(`${rootClass}-root`,className)}>
- {children}</div></CollapsibleContext.Provider>
+ >
+ <div className={clsx(`${rootClass}-root`, className)}>
+ {children}
+ </div>
+ </CollapsibleContext.Provider>
)
}📝 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 CollapsibleRoot = ({children,className="",disabled, customRootClass, open, onOpenChange}: CollapsibleRootProps) => { | |
| const rootClass = customClassSwitcher(customRootClass,COMPONENT_NAME) | |
| return ( | |
| <CollapsibleContext.Provider | |
| value={{ | |
| rootClass, | |
| open, | |
| onOpenChange, | |
| disabled | |
| }} | |
| ><div className={clsx(`${rootClass}-root`,className)}> | |
| {children}</div></CollapsibleContext.Provider> | |
| ) | |
| } | |
| const CollapsibleRoot = ({ | |
| children, | |
| className = "", | |
| disabled = false, | |
| customRootClass, | |
| open, | |
| onOpenChange | |
| }: CollapsibleRootProps) => { | |
| if (typeof open !== 'boolean') { | |
| throw new Error('open prop must be a boolean'); | |
| } | |
| const rootClass = customClassSwitcher(customRootClass,COMPONENT_NAME) | |
| return ( | |
| <CollapsibleContext.Provider | |
| value={{ | |
| rootClass, | |
| open, | |
| onOpenChange, | |
| disabled | |
| }} | |
| > | |
| <div className={clsx(`${rootClass}-root`, className)}> | |
| {children} | |
| </div> | |
| </CollapsibleContext.Provider> | |
| ) | |
| } |
🧰 Tools
🪛 eslint
[error] 20-20: A space is required after '{'.
(object-curly-spacing)
[error] 20-20: Operator '=' must be spaced.
(space-infix-ops)
[error] 20-20: A space is required before '}'.
(object-curly-spacing)
[error] 20-22: Block must not be padded by blank lines.
(padded-blocks)
| const Collapsible = ({ children, items, ...props }: CollapsibleProps) => { | ||
| //State values if not provided by the user | ||
| const [open, onOpenChange] = useState(props.open ?? true); | ||
|
|
||
|
|
||
| // Disable or enable collapse | ||
| const disabled = props.disabled ?? false; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider default open state
The component defaults to being open (true) which might be unexpected for users. Consider defaulting to closed (false) as it's a more common pattern for collapsible components.
- const [open, onOpenChange] = useState(props.open ?? true);
+ const [open, onOpenChange] = useState(props.open ?? false);📝 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 = ({ children, items, ...props }: CollapsibleProps) => { | |
| //State values if not provided by the user | |
| const [open, onOpenChange] = useState(props.open ?? true); | |
| // Disable or enable collapse | |
| const disabled = props.disabled ?? false; | |
| const Collapsible = ({ children, items, ...props }: CollapsibleProps) => { | |
| //State values if not provided by the user | |
| const [open, onOpenChange] = useState(props.open ?? false); | |
| // Disable or enable collapse | |
| const disabled = props.disabled ?? false; | |
🧰 Tools
🪛 eslint
[error] 38-39: More than 1 blank line not allowed.
(no-multiple-empty-lines)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (1)
32-32: Clarify disabled prop usageThe
disabledprop is implicitly set totrue. For a default story, consider explicitly setting it tofalseor removing it entirely to demonstrate the default collapsible behavior.- <Collapsible items={Items} disabled></Collapsible> + <Collapsible items={Items}></Collapsible>src/components/ui/Collapsible/Collapsible.tsx (1)
57-62: Improve code formatting and commentsThe trigger section could be more readable with better spacing and comments.
- {/* Button */} - - <CollapsibleTrigger asChild> - {props.trigger && props.trigger} - </CollapsibleTrigger> - + {/* Custom trigger or default button */} + <CollapsibleTrigger asChild> + {props.trigger} + </CollapsibleTrigger>src/components/ui/Collapsible/tests/Collapsible.test.tsx (1)
62-70: Fix formatting issues.Remove extra blank lines between test suites to comply with the project's formatting rules.
}); }); - - - describe("CollapsibleHeader Component", () => {🧰 Tools
🪛 eslint
[error] 62-65: Block must not be padded by blank lines.
(padded-blocks)
[error] 64-65: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 67-70: More than 1 blank line not allowed.
(no-multiple-empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx(1 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(1 hunks)src/components/ui/Collapsible/tests/Collapsible.test.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx
[error] 6-7: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 52-54: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 54-54: A space is required after '{'.
(object-curly-spacing)
[error] 54-54: A space is required before '}'.
(object-curly-spacing)
[error] 55-55: A space is required after '{'.
(object-curly-spacing)
[error] 55-55: A space is required before '}'.
(object-curly-spacing)
[error] 79-79: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx
[error] 10-10: Unexpected trailing comma.
(comma-dangle)
[error] 14-14: Unexpected trailing comma.
(comma-dangle)
[error] 17-17: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/Collapsible.tsx
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
[error] 38-39: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 66-66: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 66-71: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/tests/Collapsible.test.tsx
[error] 62-65: Block must not be padded by blank lines.
(padded-blocks)
[error] 64-65: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 67-70: More than 1 blank line not allowed.
(no-multiple-empty-lines)
🔇 Additional comments (4)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (1)
56-67:
Remove conflicting state management props
The component is receiving both controlled (open/onOpenChange) and uncontrolled (defaultOpen) props, which can lead to unexpected behavior.
Choose either controlled or uncontrolled state management:
<Collapsible
open={open}
onOpenChange={setOpen}
title="Quote"
items={Items}
- defaultOpen={Items[0]}
trigger={
<Button style={{ margin: "0" }}>
{open ? "CLOSE" : "OPEN"}
</Button>
}
/>Likely invalid or redundant comment.
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (1)
54-57: 🛠️ Refactor suggestion
Improve error handling and simplify toggle logic
The context values are destructured without validation, and the toggle logic could be simplified.
Apply this diff:
-const {rootClass,open,onOpenChange,disabled} = useContext(CollapsibleContext)
-const toggleCollapse = () => onOpenChange && !disabled && onOpenChange((p) => (!p))
+const {rootClass, open, onOpenChange, disabled} = useContext(CollapsibleContext)
+if (!rootClass) {
+ throw new Error('CollapsibleTrigger must be used within a CollapsibleRoot')
+}
+const toggleCollapse = () => !disabled && onOpenChange?.((p) => !p)Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 54-54: A space is required after '{'.
(object-curly-spacing)
[error] 54-54: A space is required before '}'.
(object-curly-spacing)
[error] 55-55: A space is required after '{'.
(object-curly-spacing)
[error] 55-55: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Collapsible/Collapsible.tsx (1)
66-86: 🛠️ Refactor suggestion
Improve items rendering implementation
The current implementation has several issues:
- Using array indices as keys
- Complex conditional rendering
- Unnecessary fragment wrapper
Apply this diff:
- {disabled ? (
- items.map((item, index) => (
- <CollapsibleItem key={index}>{item.content}</CollapsibleItem>
- ))
- ) : (
- <>
- {defaultOpen && (
- <CollapsibleItem>{defaultOpen.content}</CollapsibleItem>
- )}
- <CollapsibleContent state={props.open ?? open}>
- {items.map((item, index) => (
- item !== defaultOpen && (
- <CollapsibleItem key={index}>{item.content}</CollapsibleItem>
- )
- ))}
- </CollapsibleContent>
- </>
- )}
+ {disabled ? (
+ items.map((item) => (
+ <CollapsibleItem key={item.id}>{item.content}</CollapsibleItem>
+ ))
+ ) : (
+ <CollapsibleContent state={props.open ?? open}>
+ {defaultOpen && (
+ <CollapsibleItem key={defaultOpen.id}>
+ {defaultOpen.content}
+ </CollapsibleItem>
+ )}
+ {items
+ .filter(item => item.id !== defaultOpen?.id)
+ .map(item => (
+ <CollapsibleItem key={item.id}>
+ {item.content}
+ </CollapsibleItem>
+ ))}
+ </CollapsibleContent>
+ )}Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 66-66: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 66-71: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/tests/Collapsible.test.tsx (1)
1-9: LGTM! Imports are well-organized.
All necessary imports are present and properly organized.
| export type CollapsibleProps = { | ||
| open?: boolean; | ||
| title?: string; | ||
| trigger?: ReactNode; | ||
| items: { content: string | ReactNode }[]; | ||
| disabled?: boolean; | ||
| defaultOpen?: { content: string | ReactNode }; | ||
| onOpenChange?: Dispatch<SetStateAction<boolean>>; | ||
| } & PropsWithChildren; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and prevent prop conflicts
The current type definition allows mixing controlled and uncontrolled props, and items lack unique identifiers.
Apply these changes:
- Add unique identifiers to items:
export type CollapsibleProps = {
open?: boolean;
title?: string;
trigger?: ReactNode;
- items: { content: string | ReactNode }[];
+ items: { id: string; content: string | ReactNode }[];
disabled?: boolean;
- defaultOpen?: { content: string | ReactNode };
+ defaultOpen?: { id: string; content: string | ReactNode };
onOpenChange?: Dispatch<SetStateAction<boolean>>;
} & PropsWithChildren;- Consider splitting into controlled and uncontrolled prop types:
type BaseCollapsibleProps = {
title?: string;
trigger?: ReactNode;
items: { id: string; content: string | ReactNode }[];
disabled?: boolean;
} & PropsWithChildren;
type ControlledCollapsibleProps = BaseCollapsibleProps & {
open: boolean;
onOpenChange: Dispatch<SetStateAction<boolean>>;
defaultOpen?: never;
};
type UncontrolledCollapsibleProps = BaseCollapsibleProps & {
open?: never;
onOpenChange?: never;
defaultOpen?: { id: string; content: string | ReactNode };
};
export type CollapsibleProps = ControlledCollapsibleProps | UncontrolledCollapsibleProps;
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx (2)
21-21: Simplify toggle collapse logicThe current implementation has multiple conditions that could be simplified using optional chaining.
- const toggleCollapse = () => onOpenChange && !disabled && onOpenChange((p) => (!p)) + const toggleCollapse = () => !disabled && onOpenChange?.((p) => !p)
25-25: Remove redundant role attributeSince you're using
Primitive.button, the role="button" attribute is redundant as buttons have this role by default.- role={"button"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/fragments/CollapsibleContent.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx(1 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(1 hunks)src/components/ui/Collapsible/tests/Collapsible.test.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 13-13: A space is required after '{'.
(object-curly-spacing)
[error] 13-13: A space is required before '}'.
(object-curly-spacing)
[error] 19-19: Operator '?' must be spaced.
(space-infix-ops)
[error] 26-26: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 19-19: Unexpected trailing comma.
(comma-dangle)
[error] 22-22: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Unexpected trailing comma.
(comma-dangle)
[error] 28-28: Unexpected trailing comma.
(comma-dangle)
[error] 78-79: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 97-97: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/Collapsible.tsx
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
[error] 38-39: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 48-50: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 68-68: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 68-73: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 76-76: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 76-76: Operator '!=' must be spaced.
(space-infix-ops)
[error] 76-78: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 82-82: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 82-82: Operator '!=' must be spaced.
(space-infix-ops)
[error] 82-82: Operator '?' must be spaced.
(space-infix-ops)
[error] 82-84: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
src/components/ui/Collapsible/fragments/CollapsibleTrigger.tsx
[error] 6-7: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 17-18: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 18-18: A space is required after '{'.
(object-curly-spacing)
[error] 18-18: A space is required before '}'.
(object-curly-spacing)
[error] 19-19: A space is required after '{'.
(object-curly-spacing)
[error] 19-19: A space is required before '}'.
(object-curly-spacing)
[error] 43-43: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/tests/Collapsible.test.tsx
[error] 61-64: Block must not be padded by blank lines.
(padded-blocks)
[error] 63-64: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 66-69: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 87-88: More than 1 blank line not allowed.
(no-multiple-empty-lines)
🔇 Additional comments (7)
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx (1)
5-9:
Add missing state prop to CollapsibleContentProps type
The type definition is missing the state prop that was mentioned in the AI summary. This prop should be used to control the visibility of content.
type CollapsibleContentProps = {
children: React.ReactNode;
className?: string;
+ state?: boolean;
}Likely invalid or redundant comment.
src/components/ui/Collapsible/Collapsible.tsx (2)
28-28: Add unique identifiers to items type
Using content as implicit identifier could cause issues with duplicate content. This was previously mentioned in past reviews.
71-71: Avoid using array indices as keys
Using array indices as keys can lead to issues with item reordering or dynamic content. This was previously mentioned in past reviews.
Also applies to: 83-83
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (2)
12-24: Add unique identifiers to items array
Each item in the array should have a unique identifier for stable React keys and proper item tracking.
Apply this diff:
const Items = [
{
+ id: "quote-1",
content:
""One of the penalties for refusing to participate in politics is that you end up being governed by your inferiors." – Plato",
},
{
+ id: "quote-2",
content:
""The superior man understands what is right; the inferior man understands what will sell." – Confucius",
},
{
+ id: "quote-3",
content: ""There are no secrets on the internet." – Paul Babicki",
},
];🧰 Tools
🪛 eslint
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 19-19: Unexpected trailing comma.
(comma-dangle)
[error] 22-22: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Unexpected trailing comma.
(comma-dangle)
37-37: Verify disabled state behavior
The story demonstrates the disabled state, but it would be helpful to verify that:
- The collapsible content is not toggleable
- Visual indicators reflect the disabled state
src/components/ui/Collapsible/tests/Collapsible.test.tsx (2)
9-65: Add test coverage for missing functionality
While the existing tests cover basic functionality, please add test cases for:
onOpenChangecallback to verify state changes- External state management using custom useState values
Example additional test:
it("calls onOpenChange when state changes", () => {
const onOpenChange = jest.fn();
const { getByText } = render(
<Collapsible
trigger={<button>Toggle</button>}
items={[{ content: "Item 1" }]}
onOpenChange={onOpenChange}
/>
);
fireEvent.click(getByText("Toggle"));
expect(onOpenChange).toHaveBeenCalledWith(true);
});🧰 Tools
🪛 eslint
[error] 61-64: Block must not be padded by blank lines.
(padded-blocks)
[error] 63-64: More than 1 blank line not allowed.
(no-multiple-empty-lines)
69-97: Enhance fragment component test coverage
The current tests for fragment components are minimal. Consider adding:
- Props testing
- Interaction testing
- Accessibility testing
Example accessibility test:
describe("CollapsibleHeader Component", () => {
it("has correct ARIA attributes", () => {
const { getByRole } = render(
<CollapsibleHeader title="Test" />
);
expect(getByRole("heading")).toHaveAttribute("aria-expanded");
});
});🧰 Tools
🪛 eslint
[error] 87-88: More than 1 blank line not allowed.
(no-multiple-empty-lines)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/components/ui/Collapsible/index.tsx (1)
7-8: Remove extra blank line.
There is more than one blank line, which may violate your lint rule (no-multiple-empty-lines).- const CollapsibleComponent = {🧰 Tools
🪛 eslint
[error] 7-8: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx (1)
26-26: Add a newline at EOF.
Some tooling may expect a trailing newline for consistent formatting.🧰 Tools
🪛 eslint
[error] 26-26: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (2)
8-20: Items array is well structured.
Including a content property for each item is more flexible than passing raw text. Consider adding unique keys if mapped.🧰 Tools
🪛 eslint
[error] 11-11: Unexpected trailing comma.
(comma-dangle)
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 19-19: Unexpected trailing comma.
(comma-dangle)
50-72: Using controlled/open state.
Defining thetoggleHiddenfunction is okay, though it’s overshadowed bytrigger. If you’re deferring the actual open/close logic to Collapsible, you could remove toggleHidden.src/components/ui/Collapsible/Collapsible.tsx (2)
31-44: Improve state management and code organizationThe current implementation mixes controlled and uncontrolled state management, which could lead to confusion.
- Consider using a custom hook for state management:
function useCollapsibleState(props: CollapsibleProps) { const [internalOpen, setInternalOpen] = useState(false); const isControlled = props.open !== undefined; const open = isControlled ? props.open : internalOpen; const onOpenChange = isControlled ? props.onOpenChange : setInternalOpen; return { open, onOpenChange }; }
- Clean up the code organization:
- //State values if not provided by the user - const [open, onOpenChange] = useState(props.open ?? false); - - - // Disable or enable collapse - const disabled = props.disabled ?? false; - - // Title for the component - const title = props.title; - - // Default Value to show - const defaultPos = props.defaultPos; - - - + const { open, onOpenChange } = useCollapsibleState(props); + const disabled = props.disabled ?? false; + const title = props.title; + const defaultPos = props.defaultPos;🧰 Tools
🪛 eslint
[error] 35-36: More than 1 blank line not allowed.
(no-multiple-empty-lines)
90-97: Clean up formattingRemove extra blank lines between exports for consistent formatting.
- - Collapsible.Root = CollapsibleComponent.Root; Collapsible.Header = CollapsibleComponent.Header; Collapsible.Trigger = CollapsibleComponent.Trigger; Collapsible.Content = CollapsibleComponent.Content; Collapsible.Item = CollapsibleComponent.Item;🧰 Tools
🪛 eslint
[error] 90-92: More than 1 blank line not allowed.
(no-multiple-empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/fragments/CollapsibleContent.tsx(1 hunks)src/components/ui/Collapsible/fragments/CollapsibleItem.tsx(1 hunks)src/components/ui/Collapsible/index.tsx(1 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Collapsible/index.tsx
[error] 7-8: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 13-13: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 13-13: A space is required after '{'.
(object-curly-spacing)
[error] 13-13: A space is required before '}'.
(object-curly-spacing)
[error] 19-19: Operator '?' must be spaced.
(space-infix-ops)
[error] 26-26: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/Collapsible.tsx
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
[error] 35-36: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 45-47: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 65-65: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 65-70: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 73-73: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 73-73: Operator '!=' must be spaced.
(space-infix-ops)
[error] 73-75: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 79-79: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 79-79: Operator '!=' must be spaced.
(space-infix-ops)
[error] 79-79: Operator '?' must be spaced.
(space-infix-ops)
[error] 79-81: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 90-92: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Collapsible/fragments/CollapsibleItem.tsx
[error] 13-13: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx
[error] 11-11: Unexpected trailing comma.
(comma-dangle)
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 19-19: Unexpected trailing comma.
(comma-dangle)
[error] 24-24: Unexpected trailing comma.
(comma-dangle)
[error] 74-75: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 93-93: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (16)
src/components/ui/Collapsible/index.tsx (3)
1-5: Prefer grouped or named imports if relevant.
All imports look fine if they're individually needed. Consider grouping if sub-fragments come from the same directory.
16-16: Exporting the aggregated component is good.
Exporting a composed object with each subcomponent can improve discoverability and consistency for consumers of this library.
8-14: Validate trailing comma usage for consistent style.
Line 13 might trigger a lint error for an unexpected trailing comma. Consider removing it if your style guidelines disallow trailing commas in object properties.
- Item: CollapsibleItem,
+ Item: CollapsibleItem🧰 Tools
🪛 eslint
[error] 13-13: Unexpected trailing comma.
(comma-dangle)
src/components/ui/Collapsible/fragments/CollapsibleItem.tsx (5)
1-3: Imports confirmed.
Using clsx and React, plus the CollapsibleContext, is appropriate for this new fragment.
5-9: Type definitions look clean.
Defining a separate prop type fosters self-documenting code.
11-15: Validate context existence.
If the component is used outside CollapsibleContext, rootClass might be undefined. Consider warning or usage checks if feasible.
🧰 Tools
🪛 eslint
[error] 13-13: Unexpected trailing comma.
(comma-dangle)
18-21: Rendering is straightforward.
Labeling the parent div with a BEM-style class suffix (“-item”) is a good pattern.
23-23: Default export is correct.
Straightforward approach to distributing this fragment.
src/components/ui/Collapsible/fragments/CollapsibleContent.tsx (3)
1-10: Props typed properly.
The CollapsibleContentProps type is well-defined. Optional className with a default is good.
11-13: Context usage check.
Retrieving rootClass and open from context is convenient. However, be mindful of usage outside CollapsibleRoot. (Similar to past comment on type safety in older commits.)
🧰 Tools
🪛 eslint
[error] 11-11: A space is required after '{'.
(object-curly-spacing)
[error] 11-11: Operator '=' must be spaced.
(space-infix-ops)
[error] 11-11: A space is required before '}'.
(object-curly-spacing)
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 13-13: A space is required after '{'.
(object-curly-spacing)
[error] 13-13: A space is required before '}'.
(object-curly-spacing)
15-23: Conditional rendering is well-done.
Utilizing open && children ensures the content is only rendered if expanded, reducing DOM complexity.
🧰 Tools
🪛 eslint
[error] 19-19: Operator '?' must be spaced.
(space-infix-ops)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (5)
1-6: Dependencies check.
Using Collapsible and a local Button component is correct for showcasing stories.
23-24: Story metadata is standard.
Component and title declarations align with Storybook conventions.
🧰 Tools
🪛 eslint
[error] 24-24: Unexpected trailing comma.
(comma-dangle)
30-36: Demonstrate disabled property.
Using the disabled prop here clarifies how the collapsible behaves when inactive.
40-46: Titled collapsible usage.
Providing a title prop fairly illustrates how to show a heading for the collapsible content.
75-93: 🛠️ Refactor suggestion
Composable approach is flexible.
Exposing .Root, .Header, .Trigger, etc., fosters a more powerful usage pattern. But ensure you add unique keys when mapping over arrays in line 88.
- <Collapsible.Content>{Items.map((item) => item != Items[0] && <CollapsibleItem>{item.content}</CollapsibleItem>)}</Collapsible.Content>
+ <Collapsible.Content>
+ {Items.map((item, i) =>
+ item !== Items[0] && (
+ <CollapsibleItem key={`collapsible-item-${i}`}>{item.content}</CollapsibleItem>
+ )
+ )}
+ </Collapsible.Content>Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 93-93: Newline required at end of file but not found.
(eol-last)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/components/ui/Collapsible/tests/Collapsible.test.tsx (3)
22-35: Enhance disabled state test coverage.The current test only verifies content visibility. Consider adding assertions to:
- Verify that the trigger button is disabled or has appropriate ARIA attributes
- Check if the collapsible state remains unchanged after clicking
it("does not toggle content visibility when disabled", () => { const { getByText, queryByText } = render( <Collapsible title="Test Title" trigger={<button>Toggle</button>} disabled> <div>Test Content</div> </Collapsible> ); const triggerButton = getByText("Toggle"); + expect(triggerButton).toHaveAttribute('aria-disabled', 'true'); fireEvent.click(triggerButton); expect(queryByText("Test Content")).toBeInTheDocument(); fireEvent.click(triggerButton); expect(queryByText("Test Content")).toBeInTheDocument(); + expect(triggerButton).toHaveAttribute('aria-expanded', 'true'); });
40-45: Add comprehensive header component tests.The current test only verifies basic content rendering. Consider adding test cases for:
- Title prop rendering
- ARIA attributes for accessibility
- Integration with trigger component
describe("CollapsibleHeader Component", () => { it("renders the header content", () => { const { getByText } = render(<CollapsibleHeader>Header Content</CollapsibleHeader>); expect(getByText("Header Content")).toBeInTheDocument(); }); it("renders with title prop", () => { const { getByRole } = render(<CollapsibleHeader title="Test Title">Content</CollapsibleHeader>); const heading = getByRole("heading"); expect(heading).toHaveTextContent("Test Title"); }); it("has correct ARIA attributes", () => { const { container } = render(<CollapsibleHeader title="Test">Content</CollapsibleHeader>); expect(container.firstChild).toHaveAttribute("aria-level", "3"); }); });
47-59: Enhance fragment components test coverage.The current tests for CollapsibleItem and CollapsibleTrigger are minimal. Consider adding:
For CollapsibleTrigger:
- Click handler tests
- Disabled state tests
- Custom component forwarding tests using
asChildFor CollapsibleItem:
- Styling tests
- Nested content tests
// CollapsibleTrigger tests it("handles click events correctly", () => { const onClickMock = jest.fn(); const { getByRole } = render( <CollapsibleTrigger onClick={onClickMock}> Click me </CollapsibleTrigger> ); fireEvent.click(getByRole("button")); expect(onClickMock).toHaveBeenCalled(); }); // CollapsibleItem tests it("renders nested content correctly", () => { const { getByTestId } = render( <CollapsibleItem> <div data-testid="nested">Nested Content</div> </CollapsibleItem> ); expect(getByTestId("nested")).toBeInTheDocument(); });🧰 Tools
🪛 eslint
[error] 59-59: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/Collapsible.tsx (1)
63-76: Simplify conditional rendering logic.The current implementation uses nested ternaries and has duplicate content rendering. Consider restructuring for better readability.
- {disabled ? ( - <>{children && children} - {collapsibleContent && collapsibleContent}</> - ) : ( - <> - {children && children} - <CollapsibleComponent.Content> - {collapsibleContent && collapsibleContent} - </CollapsibleComponent.Content> - </> - )} + {children} + {disabled ? ( + collapsibleContent + ) : ( + <CollapsibleComponent.Content> + {collapsibleContent} + </CollapsibleComponent.Content> + )}🧰 Tools
🪛 eslint
[error] 63-63: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 63-67: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Collapsible/Collapsible.tsx(2 hunks)src/components/ui/Collapsible/stories/Collapsible.stories.tsx(1 hunks)src/components/ui/Collapsible/tests/Collapsible.test.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Collapsible/Collapsible.tsx
[error] 6-6: Unexpected trailing comma.
(comma-dangle)
[error] 29-30: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 37-38: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 63-63: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 63-67: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 81-83: More than 1 blank line not allowed.
(no-multiple-empty-lines)
src/components/ui/Collapsible/stories/Collapsible.stories.tsx
[error] 11-11: Unexpected trailing comma.
(comma-dangle)
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 19-19: Unexpected trailing comma.
(comma-dangle)
[error] 24-24: Unexpected trailing comma.
(comma-dangle)
[error] 115-116: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 137-137: Newline required at end of file but not found.
(eol-last)
src/components/ui/Collapsible/tests/Collapsible.test.tsx
[error] 20-22: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 35-38: Block must not be padded by blank lines.
(padded-blocks)
[error] 37-38: More than 1 blank line not allowed.
(no-multiple-empty-lines)
[error] 59-59: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (4)
src/components/ui/Collapsible/Collapsible.tsx (1)
83-87: LGTM! Clean component exports.
The component exports follow a good pattern, making the API clear and consistent.
src/components/ui/Collapsible/stories/Collapsible.stories.tsx (3)
8-20: 🛠️ Refactor suggestion
Add unique identifiers to items.
Items should have unique identifiers for stable React keys and better maintainability.
const Items = [
{
+ id: 'quote-1',
content:
""One of the penalties for refusing to participate in politics is that you end up being governed by your inferiors." – Plato",
},
{
+ id: 'quote-2',
content:
""The superior man understands what is right; the inferior man understands what will sell." – Confucius",
},
{
+ id: 'quote-3',
content: ""There are no secrets on the internet." – Paul Babicki",
},
];Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 11-11: Unexpected trailing comma.
(comma-dangle)
[error] 15-15: Unexpected trailing comma.
(comma-dangle)
[error] 18-18: Unexpected trailing comma.
(comma-dangle)
[error] 19-19: Unexpected trailing comma.
(comma-dangle)
132-132: 🛠️ Refactor suggestion
Improve map implementation in CustomTrigger story.
The current implementation has multiple issues:
- Uses loose comparison operator
- Missing key prop
- Complex inline conditional
- <Collapsible.Content>{Items.map((item) => item != Items[0] && <CollapsibleItem>{item.content}</CollapsibleItem>)}</Collapsible.Content>
+ <Collapsible.Content>
+ {Items.filter(item => item !== Items[0]).map(item => (
+ <CollapsibleItem key={item.id}>
+ {item.content}
+ </CollapsibleItem>
+ ))}
+ </Collapsible.Content>Likely invalid or redundant comment.
80-80: 🛠️ Refactor suggestion
Remove unused function.
The toggleHidden function is defined but never used since the open state is managed by the Collapsible component's onOpenChange prop.
- const toggleHidden = () => setOpen((p) => !p);Likely invalid or redundant comment.
| export type CollapsibleProps = { | ||
| open?: boolean; | ||
| title?: string; | ||
| trigger?: ReactNode; | ||
|
|
||
| disabled?: boolean; | ||
| collapsibleContent?: ReactNode; | ||
|
|
||
|
|
||
| onOpenChange?: Dispatch<SetStateAction<boolean>>; | ||
| } & PropsWithChildren; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for controlled and uncontrolled states.
The current type definition allows mixing controlled and uncontrolled props, which could lead to unexpected behavior.
type BaseCollapsibleProps = {
title?: string;
trigger?: ReactNode;
disabled?: boolean;
collapsibleContent?: ReactNode;
} & PropsWithChildren;
type ControlledCollapsibleProps = BaseCollapsibleProps & {
open: boolean;
onOpenChange: Dispatch<SetStateAction<boolean>>;
};
type UncontrolledCollapsibleProps = BaseCollapsibleProps & {
open?: never;
onOpenChange?: never;
};
export type CollapsibleProps = ControlledCollapsibleProps | UncontrolledCollapsibleProps;🧰 Tools
🪛 eslint
[error] 29-30: More than 1 blank line not allowed.
(no-multiple-empty-lines)
| {Items.map((item, index) => ( | ||
| <CollapsibleItem key={index}>{item.content}</CollapsibleItem> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array indices as keys.
Using array indices as keys can lead to rendering issues if items are reordered or modified.
- {Items.map((item, index) => (
- <CollapsibleItem key={index}>{item.content}</CollapsibleItem>
+ {Items.map((item) => (
+ <CollapsibleItem key={item.id}>{item.content}</CollapsibleItem>
))}📝 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.
| {Items.map((item, index) => ( | |
| <CollapsibleItem key={index}>{item.content}</CollapsibleItem> | |
| ))} | |
| {Items.map((item) => ( | |
| <CollapsibleItem key={item.id}>{item.content}</CollapsibleItem> | |
| ))} |
g-charan
force-pushed
the
improve-collapsible-component
branch
from
8ab0864 to
91ba422
Compare
Added changes to the collapsible component
Summary by CodeRabbit
New Features
Collapsiblecomponent with new properties for improved configurability and dynamic rendering.CollapsibleContent,CollapsibleItem,CollapsibleRoot,CollapsibleTrigger, andCollapsibleHeaderfor better structure and usability.Bug Fixes
Style
Documentation
Collapsiblecomponent to reflect new data structures and props.Tests
Collapsiblecomponent and its associated fragments to ensure correct rendering and functionality.