Add Accordion, AccordionSet components 🪗 #1943
Conversation
akdetrick
force-pushed
the
feat/Accordion
branch
from
ea2c3f2 to
4dafff3
Compare
feat(Accordion): add single accordion component
akdetrick
force-pushed
the
feat/Accordion
branch
from
4dafff3 to
0317220
Compare
akdetrick
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces two new "headless" layout components (Accordion and AccordionSet) to the Narmi Design System, providing flexible accordion UI patterns where consumers control the presentation through render props. The components support both controlled and uncontrolled modes, with AccordionSet offering exclusive (one-at-a-time) or native (multiple-open) behaviors.
Key Changes
- Added
Accordioncomponent withrenderSummaryrender prop for customizable triggers and content - Added
AccordionSetwrapper component to coordinate multiple accordions with exclusive or native behavior modes - Implemented accessibility features including ARIA attributes (aria-expanded, aria-controls, aria-hidden) and proper button semantics
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Accordion/index.tsx | Core accordion component with controlled/uncontrolled state management and accessibility attributes |
| src/Accordion/index.scss | CSS transitions using max-height and interpolate-size for smooth expand/collapse animations |
| src/Accordion/index.stories.tsx | Comprehensive Storybook examples demonstrating custom styling, focusable content, and controlled mode |
| src/AccordionSet/index.tsx | Wrapper component managing multiple accordions with exclusive or native behavior modes |
| src/AccordionSet/index.stories.tsx | Storybook examples showing exclusive/native modes and custom styling patterns |
| src/index.ts | Added TypeScript exports for Accordion and AccordionSet components |
| src/index.js | Added JavaScript exports for Accordion and AccordionSet components |
| src/index.scss | Imported Accordion SCSS for component styling |
| // In native mode, let each accordion manage its own state | ||
| const shouldControlAccordion = behavior === "exclusive"; | ||
| const isThisAccordionOpen = openAccordionIndex === index; | ||
|
|
||
| const handleToggle = (newIsOpen: boolean) => { | ||
| if (shouldControlAccordion) { | ||
| setOpenAccordionIndex(newIsOpen ? index : null); | ||
| } | ||
| // Fire callback even if uncontrolled in an AccordionSet | ||
| child.props.onUserToggle?.(newIsOpen); | ||
| }; | ||
|
|
||
| return React.cloneElement(child, { | ||
| key: index, | ||
| ...(shouldControlAccordion && { | ||
| isOpen: isThisAccordionOpen, | ||
| onUserToggle: handleToggle, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
[nitpick] In "native" mode, the AccordionSet doesn't add any behavior beyond wrapping accordions in a div. The handleToggle function is only passed to children in exclusive mode (line 58-61). In native mode, each Accordion already has its own uncontrolled state management. Consider documenting when and why to use AccordionSet with "native" mode, or if it provides semantic/accessibility benefits beyond a plain div wrapper.
| return React.cloneElement(child, { | ||
| key: index, | ||
| ...(shouldControlAccordion && { | ||
| isOpen: isThisAccordionOpen, | ||
| onUserToggle: handleToggle, | ||
| }), |
There was a problem hiding this comment.
I am not familiar with cloneElement but it seems like a smart use case. Does this link all of the child instances of onUserToggle so that exclusive mode opening one of the children will close all others?
There was a problem hiding this comment.
Exactly - cloneElement is basically a way to intercept a react child and modify its props.
AccordionSet can use this to manage "exclusive" mode, where only one child opens at a time
|
@akdetrick I've opened a new pull request, #1944, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
✅ No visual changes detected📘 Storybook previewThis PR is posted and updated automatically by the |
Co-authored-by: akdetrick <231252+akdetrick@users.noreply.github.com>
[WIP] Update Accordion components to use useId for labelledby attribute
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
✅ No visual changes detected📘 Storybook previewThis PR is posted and updated automatically by the |
✅ No visual changes detected📘 Storybook previewThis PR is posted and updated automatically by the |
✅ No visual changes detected📘 Storybook previewThis PR is posted and updated automatically by the |
✅ No visual changes detected📘 Storybook previewThis PR is posted and updated automatically by the |
graphicsforge
left a comment
graphicsforge
left a comment
There was a problem hiding this comment.
This makes a lot of sense to me, we have a lot of added customizability with the renderSummary prop getting the isOpen state, and the exclusive AccordionSet allows us to manage relationships between multiple Accordions.
Closes NDS-2059
Introduces "headless" accordion layout components. Engineers may pass arbitrary JSX to the
summary(trigger) and thecontentto create any style of accordion-like UI.