feat: custom-accordion-expand-button #85
Conversation
✅ Deploy Preview for kleros-v2-ui-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughExports accordion item types, adds a customizable per-item or global Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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 (3)
src/lib/accordion/index.tsx (1)
46-48: Simplify the negated condition.Per SonarCloud's hint, the negated condition can be inverted for better readability.
const [expanded, setExpanded] = useState( - !isUndefined(defaultExpanded) ? defaultExpanded : -1, + isUndefined(defaultExpanded) ? -1 : defaultExpanded, );src/lib/accordion/custom.tsx (1)
58-60: Simplify the negated condition.Same as in
index.tsx, invert the condition for clarity.const [expanded, setExpanded] = useState( - !isUndefined(defaultExpanded) ? defaultExpanded : -1, + isUndefined(defaultExpanded) ? -1 : defaultExpanded, );src/lib/accordion/accordion-item.tsx (1)
60-77: Consider extracting nested ternary for readability.Per SonarCloud's suggestion, the nested ternary could be refactored into a clearer structure.
const ExpandButton = useMemo(() => { + if (expandButton) { + return expandButton({ + expanded, + toggle: () => setExpanded(expanded ? -1 : index), + }); + } + const IconComponent = expanded ? Minus : Plus; + return ( + <IconComponent + className={cn("fill-klerosUIComponentsPrimaryText size-4 shrink-0")} + /> + ); - expandButton ? ( - expandButton({ - expanded, - toggle: () => setExpanded(expanded ? -1 : index), - }) - ) : expanded ? ( - <Minus - className={cn("fill-klerosUIComponentsPrimaryText size-4 shrink-0")} - /> - ) : ( - <Plus - className={cn("fill-klerosUIComponentsPrimaryText size-4 shrink-0")} - /> - ), - [expanded, expandButton, index, setExpanded], + }, [expanded, expandButton, index, setExpanded]); );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/accordion/accordion-item.tsx(1 hunks)src/lib/accordion/custom.tsx(2 hunks)src/lib/accordion/index.tsx(2 hunks)src/stories/accordion.stories.tsx(1 hunks)src/stories/custom-accordion.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tractorss
Repo: kleros/ui-components-library PR: 80
File: src/lib/accordion/accordion-item.tsx:28-28
Timestamp: 2025-06-06T12:21:11.794Z
Learning: In the AccordionItem component in src/lib/accordion/accordion-item.tsx, the static id="expand-button" on the Button element is intentional and should not be changed to dynamic IDs, even though it creates duplicate IDs across multiple accordion items.
📚 Learning: 2025-06-06T12:21:11.794Z
Learnt from: tractorss
Repo: kleros/ui-components-library PR: 80
File: src/lib/accordion/accordion-item.tsx:28-28
Timestamp: 2025-06-06T12:21:11.794Z
Learning: In the AccordionItem component in src/lib/accordion/accordion-item.tsx, the static id="expand-button" on the Button element is intentional and should not be changed to dynamic IDs, even though it creates duplicate IDs across multiple accordion items.
Applied to files:
src/lib/accordion/custom.tsxsrc/lib/accordion/index.tsxsrc/lib/accordion/accordion-item.tsxsrc/stories/accordion.stories.tsxsrc/stories/custom-accordion.stories.tsx
🧬 Code graph analysis (5)
src/lib/accordion/custom.tsx (3)
src/lib/accordion/accordion-item.tsx (1)
AccordionItemProps(9-49)src/lib/accordion/index.tsx (1)
AccordionItemProps(5-10)src/utils/index.ts (2)
isUndefined(8-11)cn(4-6)
src/lib/accordion/index.tsx (2)
src/lib/accordion/accordion-item.tsx (1)
AccordionItemProps(9-49)src/utils/index.ts (2)
isUndefined(8-11)cn(4-6)
src/lib/accordion/accordion-item.tsx (3)
src/lib/accordion/index.tsx (1)
AccordionItemProps(5-10)src/hooks/useElementSize.ts (1)
useElementSize(10-40)src/utils/index.ts (1)
cn(4-6)
src/stories/accordion.stories.tsx (3)
.storybook/preview.tsx (1)
IPreviewArgs(8-11)src/stories/utils.tsx (1)
IPreviewArgs(1-6)src/stories/custom-accordion.stories.tsx (1)
Accordion(20-59)
src/stories/custom-accordion.stories.tsx (1)
src/stories/accordion.stories.tsx (1)
Accordion(18-42)
🪛 GitHub Check: SonarCloud Code Analysis
src/lib/accordion/custom.tsx
[warning] 59-59: Unexpected negated condition.
src/lib/accordion/index.tsx
[warning] 47-47: Unexpected negated condition.
src/lib/accordion/accordion-item.tsx
[warning] 67-75: Extract this nested ternary operation into an independent statement.
🔇 Additional comments (4)
src/stories/accordion.stories.tsx (1)
6-18: LGTM!The import alias to
AccordionComponentand story renaming toAccordionimprove clarity and align with the updated public API structure.src/lib/accordion/custom.tsx (1)
51-82: LGTM on the expandButton fallback pattern.The
item.expandButton ?? expandButtonfallback logic cleanly allows per-item overrides while supporting a global default.src/lib/accordion/accordion-item.tsx (2)
80-93: Good accessibility improvement witharia-expanded.Adding
aria-expanded={expanded}properly communicates the accordion state to assistive technologies.
9-49: Well-documented props interface.The JSDoc comments on
AccordionItemPropsthoroughly explain theexpandButtonrender prop pattern with a clear example.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/stories/custom-accordion.stories.tsx (1)
102-102: Fix typo in story doc comment (“ovverrided” → “overridden”).The JSDoc above
ItemExpandButtonstill reads “ovverrided”; please correct for clarity:-/** Parent Expand Button can be ovverrided at Item level if required */ +/** Parent expand button can be overridden at item level if required */
🧹 Nitpick comments (4)
src/lib/accordion/index.tsx (2)
5-10: Avoid having two differentAccordionItemPropstypes in separate modules.This file defines
AccordionItemProps(stringtitle,Icon/icon), whilesrc/lib/accordion/accordion-item.tsxexports anotherAccordionItemProps(ReactNodetitle,body,expandButton,expanded, etc.). Even though they live in different modules, the identical name but different shape is easy to mis-import and misunderstand.Consider either:
- Renaming this one to something like
AccordionItemConfig(and re-exporting the other as the canonicalAccordionItemProps), or- Re-exporting the implementation-facing
AccordionItemPropsfromaccordion-item.tsxhere and using a narrower, clearly named type for the publicitemsconfig if needed.
12-25: AlignAccordionPropswith the fact that you spread...propsonto the root<div>.
Accordioncurrently has props{ items, defaultExpanded, className }but also does<div {...props}>. This means callers can pass extra attributes (e.g.id,data-*,onClick) that are forwarded at runtime but are not modeled inAccordionProps.To keep types aligned with behavior, consider:
export interface AccordionProps extends React.HTMLAttributes<HTMLDivElement> { items: AccordionItemProps[]; defaultExpanded?: number; className?: string; }or, if pass-through is not desired, drop
{...props}entirely.Also applies to: 40-45, 50-56
src/lib/accordion/accordion-item.tsx (1)
60-71: You can simplifyExpandButtonand dropuseMemofor clarity.
useMemois wrapping a cheap render helper that already depends onexpandedandexpandButton, so it offers little benefit and adds indirection. You could inline this logic and keep it easier to follow:- const ExpandButton = useMemo(() => { - if (expandButton) { - return expandButton({ - expanded, - toggle: () => setExpanded(expanded ? -1 : index), - }); - } - const IconComponent = expanded ? Minus : Plus; - return ( - <IconComponent className="fill-klerosUIComponentsPrimaryText size-4 shrink-0" /> - ); - }, [expanded, expandButton, index, setExpanded]); + const toggle = () => setExpanded(expanded ? -1 : index); + + const ExpandButton = expandButton + ? expandButton({ expanded, toggle }) + : expanded ? ( + <Minus className="fill-klerosUIComponentsPrimaryText size-4 shrink-0" /> + ) : ( + <Plus className="fill-klerosUIComponentsPrimaryText size-4 shrink-0" /> + );This keeps the same behavior with less hook complexity.
Also applies to: 86-87
src/lib/accordion/custom.tsx (1)
5-6: Model passthrough DOM props inCustomAccordionPropsor drop{...props}.Similar to
Accordion,CustomAccordionspreads...propsonto the root<div>, butCustomAccordionPropsonly definesitems,className,defaultExpanded, andexpandButton. Any extra attributes passed (e.g.id,data-*) work at runtime but are invisible to the type system.Consider:
export interface CustomAccordionProps extends React.HTMLAttributes<HTMLDivElement> { items: Pick<AccordionItemProps, "title" | "body" | "expandButton">[]; className?: string; defaultExpanded?: number; expandButton?: AccordionItemProps["expandButton"]; }or, if you don’t want arbitrary attributes forwarded, remove
{...props}from the<div>.Also applies to: 14-24, 51-57, 62-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/accordion/accordion-item.tsx(1 hunks)src/lib/accordion/custom.tsx(2 hunks)src/lib/accordion/index.tsx(2 hunks)src/stories/custom-accordion.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tractorss
Repo: kleros/ui-components-library PR: 80
File: src/lib/accordion/accordion-item.tsx:28-28
Timestamp: 2025-06-06T12:21:11.794Z
Learning: In the AccordionItem component in src/lib/accordion/accordion-item.tsx, the static id="expand-button" on the Button element is intentional and should not be changed to dynamic IDs, even though it creates duplicate IDs across multiple accordion items.
📚 Learning: 2025-06-06T12:21:11.794Z
Learnt from: tractorss
Repo: kleros/ui-components-library PR: 80
File: src/lib/accordion/accordion-item.tsx:28-28
Timestamp: 2025-06-06T12:21:11.794Z
Learning: In the AccordionItem component in src/lib/accordion/accordion-item.tsx, the static id="expand-button" on the Button element is intentional and should not be changed to dynamic IDs, even though it creates duplicate IDs across multiple accordion items.
Applied to files:
src/lib/accordion/accordion-item.tsxsrc/lib/accordion/custom.tsxsrc/stories/custom-accordion.stories.tsxsrc/lib/accordion/index.tsx
🧬 Code graph analysis (4)
src/lib/accordion/accordion-item.tsx (3)
src/lib/accordion/index.tsx (1)
AccordionItemProps(5-10)src/hooks/useElementSize.ts (1)
useElementSize(10-40)src/utils/index.ts (1)
cn(4-6)
src/lib/accordion/custom.tsx (3)
src/lib/accordion/accordion-item.tsx (1)
AccordionItemProps(9-49)src/lib/accordion/index.tsx (1)
AccordionItemProps(5-10)src/utils/index.ts (2)
isUndefined(8-11)cn(4-6)
src/stories/custom-accordion.stories.tsx (1)
src/stories/accordion.stories.tsx (1)
Accordion(18-42)
src/lib/accordion/index.tsx (2)
src/lib/accordion/accordion-item.tsx (1)
AccordionItemProps(9-49)src/utils/index.ts (2)
isUndefined(8-11)cn(4-6)
🔇 Additional comments (4)
src/lib/accordion/index.tsx (1)
12-25: Accordion API and defaultExpanded behavior look good.The updated docs on
items/defaultExpanded, theisUndefined(defaultExpanded) ? -1 : defaultExpandedinitialization, and the responsivew-full max-w-[1000px]container together make the Accordion API clearer and more robust while preserving single-item expansion semantics.Also applies to: 27-32, 46-48, 51-54
src/lib/accordion/accordion-item.tsx (1)
9-49: AccordionItemProps + expandButton API are well-structured.The public
AccordionItemPropswithtitle,body,expanded, and the documentedexpandButtoncallback (with{ expanded, toggle }) matches the custom/global button stories and keeps the header/body/transition logic coherent. The addedaria-expandedand preserved static"expand-button"id also keep behavior predictable and accessible. Based on learnings, keeping the static id is correct.Also applies to: 73-97
src/lib/accordion/custom.tsx (1)
5-43: CustomAccordionProps and expandButton override behavior look solid.Using
items: Pick<AccordionItemProps, "title" | "body" | "expandButton">[]plus a parent-levelexpandButton?: AccordionItemProps["expandButton"]neatly models both per-item and global buttons, anditem.expandButton ?? expandButtondoes the right precedence. ThedefaultExpandedhandling matches the base Accordion, keeping the mental model consistent.Also applies to: 51-60, 69-80
src/stories/custom-accordion.stories.tsx (1)
19-59: Stories accurately exercise per-item and globalexpandButtonbehavior.The three stories cover: per-item buttons, a shared global button, and per-item overrides on top of a global default. All use the correct
({ expanded, toggle })callback signature and wireonPress={toggle}on theButton, which matches theAccordionItemProps["expandButton"]API.Also applies to: 61-100, 102-148
PR-Codex overview
This PR focuses on refactoring the
Accordioncomponent to improve its props and structure, as well as introducing a newCustomAccordioncomponent that allows for more customization in rendering accordion items.Detailed summary
AccordiontoAccordionComponentinaccordion.stories.tsx.AccordionItemandAccordionPropsto includeexpandButton.AccordionPropsandCustomAccordionProps.CustomAccordionwith customizableexpandButtonfunctionality.AccordionandCustomAccordioncomponents.Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.