feat: add ExpansionPanel component#1075
Conversation
An animated expand/collapse (disclosure) primitive: ExpansionPanel, ExpansionPanelHeader, ExpansionPanelToggle, and ExpansionPanelContent. - Controlled (isExpanded + onToggleExpand) and uncontrolled (initiallyExpanded) modes; the toggle wires aria-expanded + aria-controls automatically and renders an icon-only chevron or a full-width button when given children. - Content animates height (react-transition-group) with an opacity cross-fade and a subtle drop-in. - Add react-transition-group as a peer dependency (already used by comms-web and todoist-web) and to the rollup external list. - Chevron authored as a local inline-SVG, kept in the component folder for now pending shared icon syncing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doistbot
left a comment
There was a problem hiding this comment.
This adds a well-animated ExpansionPanel disclosure primitive and consolidates duplicated logic from our apps.
Few things worth tightening:
- API surface: Make
idoptional with an internal fallback; switchisExpandedfallback from||to??; stop silently overwriting consumeronClick/startIcon; and explicitly exportExpansionPanelPropsandExpansionPanelToggleProps. - Refs & docs: Wrap
ExpansionPanelToggle,ExpansionPanelHeader, andExpansionPanelContentinReact.forwardRef; remove the redundant outerBox; and add missing JSDoc forchildrenandaria-describedby. - Performance: Add
mountOnEnter/unmountOnExitso collapsed panels unmount their subtrees, and use a stable functional updater for the uncontrolled toggle to avoid unnecessary re-renders. - Tests: Add a
jest-axea11y check, cover theButtontoggle branch with children, and assert theonEnteredcallback fires after the animation.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (3)
- [P3] src/expansion-panel/expansion-panel.tsx:39: Repository guidelines state: "Add JSDoc comments to every prop". The
childrenprop here, as well asaria-describedbyinExpansionPanelToggleProps, are missing documentation. - [P3] src/expansion-panel/expansion-panel.tsx:196: This outer
<Box>wrapper is redundant, asAnimatedExpansionPanelContentalready returns aBoxat its root. Removing it will flatten the DOM tree. - [P3] src/expansion-panel/expansion-panel.tsx:18: The inline function
() => setState(!state)capturesstateand creates a new reference on every render in uncontrolled mode (unless automatically cached by the React Compiler). BecauseonToggleExpandis included in theExpansionPanelcontext value's dependency array, an unstable reference can defeatuseMemoand force all context consumers to re-render on unrelated parent updates. Using a stable updater callback (const toggle = React.useCallback(() => setState(s => !s), [])) guarantees the reference never changes and prevents this unnecessary invalidation.
| } | ||
|
|
||
| const [isExpanded, onToggleExpand] = useControlledState( | ||
| props.isExpanded || initiallyExpanded, |
There was a problem hiding this comment.
🟡 P2 Using || causes isExpanded={false} to fall through to initiallyExpanded (which is undefined in controlled mode). This only works correctly right now because the internal useControlledState hook happens to use false as its default parameter. Change this to props.isExpanded ?? initiallyExpanded to safely preserve the explicit false state.
| * with a chevron start icon otherwise. Remaining props forward to the underlying | ||
| * button. | ||
| */ | ||
| function ExpansionPanelToggle({ |
There was a problem hiding this comment.
🟡 P2 Repository guidelines require wrapping components in React.forwardRef. This is critical for ExpansionPanelToggle so consumers can attach a ref for focus management, tooltips, or popovers. ExpansionPanelHeader and ExpansionPanelContent should also forward refs to their underlying Box components.
| width="full" | ||
| align="start" | ||
| {...props} | ||
| onClick={onToggleExpand} |
There was a problem hiding this comment.
🟡 P2 Explicit props like onClick and startIcon are placed after {...props}, which silently overwrites any values provided by the consumer. Either omit these from ExpansionPanelToggleProps (as you did for icon and size) so they cause a type error, or compose them (e.g., chain onClick handlers).
| ) | ||
| } | ||
|
|
||
| export { ExpansionPanel, ExpansionPanelContent, ExpansionPanelHeader, ExpansionPanelToggle } |
There was a problem hiding this comment.
🟡 P2 Repository guidelines require exporting types explicitly (export type { ButtonProps }). ExpansionPanelProps and ExpansionPanelToggleProps should be exported here (and subsequently from the folder's index.ts) so consumers can strongly type their implementations.
| * The id to apply to the expanded content, used by the toggle's | ||
| * aria-controls attribute | ||
| */ | ||
| id: string |
There was a problem hiding this comment.
🟡 P2 Requiring id here leaks the panel's internal aria-controls wiring into every call site. We already auto-generate stable IDs for similar relationships elsewhere, and React 18 gives us useId() for this. Making id optional and generating a fallback would avoid stringly-typed setup and accidental duplicate IDs across panels.
| }, []) | ||
|
|
||
| return ( | ||
| <Transition |
There was a problem hiding this comment.
🟡 P2 Transition is left at its default mount behavior here, so a collapsed panel still renders and keeps its entire content subtree mounted; the display: 'none' on the container only skips paint. In a list/sidebar with many collapsed panels, that eagerly runs all child renders/effects up front and keeps hidden content alive after collapse. Add mountOnEnter at minimum, and unmountOnExit too if preserving hidden state isn't required.
| @@ -0,0 +1,64 @@ | |||
| import * as React from 'react' | |||
There was a problem hiding this comment.
🟡 P2 Missing jest-axe accessibility test. The project's CLAUDE.md requires every component to have an a11y test, and every other component in the repo follows this convention. Add:
import { axe } from 'jest-axe'
it('renders with no a11y violations', async () => {
const { container } = renderPanel()
expect(await axe(container)).toHaveNoViolations()
})| | { initiallyExpanded?: boolean } | ||
| | { isExpanded: boolean; onToggleExpand: () => void } | ||
|
|
||
| function renderPanel(overrides: PanelOverrides = {}) { |
There was a problem hiding this comment.
🟡 P2 The renderPanel helper and all three tests only exercise the icon-only (IconButton) toggle path. The Button variant (when ExpansionPanelToggle receives children) is a materially different rendering branch — it renders a <Button> with startIcon instead of an <IconButton> with icon, has distinct CSS, and is one of the two documented usage patterns in the Storybook stories. No test covers it. A simple additional test that renders renderPanel with an ExpansionPanelToggle that has children would verify the toggle still functions and wires aria-expanded/aria-controls correctly in that variant.
| <ExpansionPanelHeader> | ||
| <ExpansionPanelToggle aria-label="Toggle section" /> | ||
| </ExpansionPanelHeader> | ||
| <ExpansionPanelContent> |
There was a problem hiding this comment.
🟡 P2 The onEntered callback prop on ExpansionPanelContent is part of the public API (fired when the expand animation completes) but is never exercised. Use jest.useFakeTimers() to advance past the 200 ms transition timeout and assert the callback fires after expanding.
doistbot
left a comment
There was a problem hiding this comment.
Adds a clean, animated ExpansionPanel disclosure primitive that consolidates duplicate expand/collapse behavior across projects.
Few things worth tightening:
- Fix the controlled-mode state logic:
props.isExpanded || initiallyExpandedbreaks whenisExpandedis explicitlyfalse, so use??instead. Derive controlled mode fromisExpanded !== undefinedrather than the presence ofonToggleExpand, and memoize the uncontrolled toggle callback withuseCallbackso context stability isn't lost. - Tighten the toggle API so
aria-labelis required only for the icon-only variant, internally owned props (onClick,aria-expanded,aria-controls) are omitted from the public type, consumeronClickhandlers are preserved, and the rootidis optional with an internal fallback. - Align with Reactist conventions: wrap all subcomponents in
React.forwardRef, add ajest-axeaccessibility check, explicitly export the component prop types, and move the story file from.jsxto.tsx. - Expand tests to cover the full-width
Buttontoggle variant and assert thatExpansionPanelContentis actually shown or hidden in expanded and collapsed states.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (2)
- [P3] src/expansion-panel/expansion-panel.tsx:33: Per project conventions ("Export types explicitly"), please export
ExpansionPanelProps(andExpansionPanelToggleProps, etc.) so consumers can safely type their own wrappers. - [P3] src/expansion-panel/expansion-panel.stories.jsx:1: Storybook file uses
.jsxbut the documented convention prefers.stories.mdxor.stories.tsx. Consider renaming to.stories.tsxfor consistency with the rest of the TypeScript codebase.
| ) | ||
| } | ||
|
|
||
| describe('ExpansionPanel', () => { |
There was a problem hiding this comment.
🟡 P2 Per project conventions, every component should have an accessibility test. Please add a jest-axe check to this suite (e.g. expect(await axe(container)).toHaveNoViolations()).
There was a problem hiding this comment.
Done in 8eb3669 — added a jest-axe check to the suite: const { container } = renderPanel({ initiallyExpanded: true }); expect(await axe(container)).toHaveNoViolations().
— Claude
| * (`initiallyExpanded`) modes; mixing the two logs a warning and falls back to | ||
| * controlled. | ||
| */ | ||
| function ExpansionPanel({ children, initiallyExpanded, id, ...props }: ExpansionPanelProps) { |
There was a problem hiding this comment.
🟡 P2 Per project conventions (CLAUDE.md), design system components should use React.forwardRef. Please wrap ExpansionPanel, ExpansionPanelHeader, ExpansionPanelToggle, and ExpansionPanelContent in React.forwardRef so consumers can attach refs to the underlying DOM elements if needed.
There was a problem hiding this comment.
Done in 8eb3669 — wrapped all four subcomponents (ExpansionPanel, ExpansionPanelHeader, ExpansionPanelToggle, ExpansionPanelContent) in React.forwardRef with named functions, forwarding the ref to the underlying DOM element.
— Claude
| variant="quaternary" | ||
| size="small" | ||
| {...props} | ||
| onClick={onToggleExpand} |
There was a problem hiding this comment.
🟡 P2 Placing onClick={onToggleExpand} after {...props} silently overwrites any onClick handler provided by the consumer. Consider merging them so the consumer's handler still fires (e.g. onClick={(e) => { props.onClick?.(e); onToggleExpand(); }}). This also applies to the Button on line 168.
There was a problem hiding this comment.
Done in 8eb3669 — resolved by omitting the internally-owned props (onClick, aria-expanded, aria-controls) from the public toggle type. Consumers can no longer pass onClick, so there's nothing to silently overwrite or merge.
— Claude
|
|
||
| function useControlledState(value = false, onToggle?: () => void): [boolean, () => void] { | ||
| const [state, setState] = React.useState(value) | ||
| return onToggle === undefined ? [state, () => setState(!state)] : [value, onToggle] |
There was a problem hiding this comment.
🟡 P2 In uncontrolled mode, returning () => setState(!state) creates a new function reference on every render. This defeats the React.useMemo for contextValue below, causing the context to update and re-render all consumers on every re-render of ExpansionPanel. Consider using a stable callback: React.useCallback(() => setState(s => !s), []).
There was a problem hiding this comment.
Done in 8eb3669 — the uncontrolled toggle is now a stable callback: React.useCallback(() => setInternalState((expanded) => !expanded), []), so contextValue no longer changes on every render.
— Claude
| * Required accessible label for the toggle button. The consumer owns | ||
| * localization — typically something like `"Toggle list of Projects"`. | ||
| */ | ||
| 'aria-label': string |
There was a problem hiding this comment.
🟡 P2 The toggle prop surface is inaccurate here. This top-level aria-label requirement forces text-button callers to keep a second label string even though visible children already name the button, and the inherited Button/IconButton props still allow onClick/aria-expanded/aria-controls even though those are always overwritten below. Please split the branches so only the icon-only variant requires aria-label, and omit the internally owned props from both branches.
There was a problem hiding this comment.
Done in 8eb3669 — split the union so aria-label is required only for the icon-only variant (text buttons are named by their children), and the internally-owned props (onClick/aria-expanded/aria-controls) are omitted from both branches.
— Claude
| /** Make the listed keys of `T` optional. */ | ||
| type SetOptional<T, K extends keyof T> = Omit<T, K> & Partial<Pick<T, K>> | ||
|
|
||
| function useControlledState(value = false, onToggle?: () => void): [boolean, () => void] { |
There was a problem hiding this comment.
🟡 P2 Controlled mode is derived from onToggle being present instead of from isExpanded. In JS consumers, passing isExpanded without onToggleExpand produces a hybrid component that only seeds internal state from the prop once, so later prop updates are ignored. It would be safer to decide control from isExpanded !== undefined and warn or throw when the controlled pair is incomplete.
There was a problem hiding this comment.
Done in 8eb3669 — controlled mode is now derived from isExpanded !== undefined rather than the presence of onToggleExpand, so later prop updates are honored. Also added a console.warn when isExpanded is passed without onToggleExpand.
— Claude
| * The id to apply to the expanded content, used by the toggle's | ||
| * aria-controls attribute | ||
| */ | ||
| id: string |
There was a problem hiding this comment.
🟡 P2 Requiring id on the root exposes an internal implementation detail to every caller. The only current use is wiring aria-controls to the content element, so callers now have to manufacture globally unique ids just to render a panel. Consider making this optional and generating a stable fallback internally, with an override only when a consumer needs a specific id.
There was a problem hiding this comment.
Done in 8eb3669 — id is now optional with a React.useId() fallback; consumers only pass one when they need a specific, stable DOM id.
— Claude
| | { initiallyExpanded?: boolean } | ||
| | { isExpanded: boolean; onToggleExpand: () => void } | ||
|
|
||
| function renderPanel(overrides: PanelOverrides = {}) { |
There was a problem hiding this comment.
🟡 P2 The renderPanel helper and all three test cases only exercise the icon-only toggle path (no children). The ExpansionPanelToggle component has a second rendering variant — when children are passed it renders a full-width Button with startIcon instead of an IconButton. This distinct code path with different prop forwarding and DOM structure is untested.
There was a problem hiding this comment.
Done in 8eb3669 — added renderButtonTogglePanel and a test covering the full-width Button variant (children): it asserts the button is named by its children, wires aria-controls, and toggles aria-expanded.
— Claude
| expect(document.getElementById('panel-content')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('toggles its expanded state in uncontrolled mode', async () => { |
There was a problem hiding this comment.
🟡 P2 Tests verify aria-expanded but never assert whether ExpansionPanelContent is actually shown or hidden. Add visibility assertions (e.g., toBeVisible() / not.toBeVisible()) for the initial expanded and collapsed states to catch animation regressions.
There was a problem hiding this comment.
Done in 8eb3669 — added a visibility test: content toBeVisible() when initiallyExpanded, and not.toBeVisible() when collapsed (a collapsed panel starts in the exited / display:none state, so this is assertable without advancing timers).
— Claude
| } | ||
|
|
||
| const [isExpanded, onToggleExpand] = useControlledState( | ||
| props.isExpanded || initiallyExpanded, |
There was a problem hiding this comment.
🟠 P1 props.isExpanded || initiallyExpanded evaluates to undefined when isExpanded is explicitly false (since false || undefined → undefined). This breaks aria-expanded and leaves the panel in an invalid state in controlled mode. Use props.isExpanded ?? initiallyExpanded instead.
There was a problem hiding this comment.
Done in 8eb3669 — switched to props.isExpanded ?? initiallyExpanded. (The value = false default parameter happened to coerce the false case back to false, so it wasn't an observable bug — but ?? is the correct intent.)
— Claude
|
Hey @benbreckler, thanks for tackling this. Could you please address Doistbot's comments and then re-request a review from me? |
- Wrap all subcomponents in React.forwardRef and export their prop types - Derive controlled mode from `isExpanded` (not `onToggleExpand`), use `??` for the fallback, and make the uncontrolled toggle a stable callback; warn on an incomplete controlled pair - Make `id` optional with a `useId` fallback - Toggle API: require `aria-label` only for the icon-only variant and omit the internally-owned props (onClick/aria-expanded/aria-controls) from the public type - Tests: add jest-axe a11y check, a full-width button-variant test, and content visibility assertions - Rename the story from .jsx to .tsx Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Sure, I asked AI to address the Doistbot comments. Let me know if you have feedback @frankieyan 🙏 |
frankieyan
left a comment
There was a problem hiding this comment.
Looking mostly good, Ben. Left some comments for you, once addressed I think it'd be good to go, but please ping me again when ready.
| .container { | ||
| min-height: 0; | ||
| height: 0; | ||
| transition-duration: 200ms; |
There was a problem hiding this comment.
Does the design team have tokens assigned to transition timings and easing functions? This could be a good opportunity to introduce them here.
While we have variables defined for them in Todoist, we haven't done a good job extracting these into variables here in Reactist.
There was a problem hiding this comment.
We discussed an animation framework recently but we don't have a final plan and values yet.
I would suggest we go with this approach for now and then use the variables to replace all current values later on?
|
|
||
| import styles from './animated-expansion-panel-content.module.css' | ||
|
|
||
| const HEIGHT_TRANSITION_DURATION = 200 /* milliseconds */ |
There was a problem hiding this comment.
Instead of adding a comment for the unit, it is more common that we include it in the variable name:
| const HEIGHT_TRANSITION_DURATION = 200 /* milliseconds */ | |
| const HEIGHT_TRANSITION_DURATION_MS = 200 |
There was a problem hiding this comment.
💭 Also, we do end up defining 200ms and the curves twice - once here and again in animated-expansion-panel-content.module.css. Nothing to action on for now but we should implement a way for these to be de-duped.
There was a problem hiding this comment.
Done in 20a0d64 — renamed to HEIGHT_TRANSITION_DURATION_MS. On the duplicated 200ms/curves across the TS and CSS: agreed, left as-is for now per your note; worth a follow-up to dedupe (and tokenize timings/easings more broadly).
— Claude
| return | ||
| } | ||
|
|
||
| setElementHeight(transitionElementRef.current, wrapperRef.current?.clientHeight ?? 0) |
There was a problem hiding this comment.
💭 Nothing to action on here, but hopefully, interpolate-size comes to Firefox and Safari soon so this can be done in CSS.
There was a problem hiding this comment.
Agreed, nothing to change here — would be great to move this into CSS once interpolate-size lands across browsers.
— Claude
| {(state) => ( | ||
| <Box | ||
| ref={transitionElementRef} | ||
| overflow="hidden" |
There was a problem hiding this comment.
Having overflow: hidden here could be confusing, because we're splitting up the logic by also setting overflow: visible via the .entered class. I'd consolidate it in one place:
| overflow="hidden" | |
| overflow={state === 'entered' ? 'visible' : 'hidden} |
Then, remove overflow: visible; from the .entered selector.
There was a problem hiding this comment.
Done in 20a0d64 — overflow is now driven by the Box prop (overflow={state === 'entered' ? 'visible' : 'hidden'}) and removed from the .entered selector.
— Claude
| ref={transitionElementRef} | ||
| overflow="hidden" | ||
| className={[styles.container, state === 'entered' ? styles.entered : null]} | ||
| style={state === 'exited' ? { display: 'none' } : undefined} |
There was a problem hiding this comment.
<Box> exposes display so we can express this without the style attribute:
| style={state === 'exited' ? { display: 'none' } : undefined} | |
| display={state === 'exited' ? 'none' : 'block'} |
There was a problem hiding this comment.
Done in 20a0d64 — switched to display={state === 'exited' ? 'none' : 'block'} via the Box prop, dropping the inline style.
— Claude
|
|
||
| const ChevronIcon = size === '24' ? ChevronDownIcon : ChevronDownSmallIcon | ||
|
|
||
| // `props` is the toggle's discriminated union minus the destructured keys. |
There was a problem hiding this comment.
TypeScript is fine with props below (on line 199 and 220) being spread without the as assertions, so I'd remove those + this comment. It works because TypeScript is a little loose when it comes to spreading objects as props, but if we want to properly discriminate between the Icon and Button prop types, we'll have to do the prop descructuring within and outside the if branch here, and IMO that's not worth the verbosity.
So three things:
- Remove
//propsis the toggle's.... - Replace
{...(props as ....)}in the two instances below with{...props}
There was a problem hiding this comment.
Done in 20a0d64 — removed the as casts and the comment; plain {...props} type-checks, as you said.
— Claude
| * expand/collapse driven by the parent `ExpansionPanel`'s state. Extra props | ||
| * forward to the inner `Box`. | ||
| */ | ||
| const ExpansionPanelContent = React.forwardRef<HTMLDivElement, ExpansionPanelContentProps>( |
There was a problem hiding this comment.
I don't think we need to expose ref from these components. What ref does is expose the HTML element for further manipulation beyond the exposed/documented API (the props). I don't think we should allow this flexibility unless we find a good reason to.
So, please remove the wrapp const ExpansionPanelContent = React.forwardRef<HTMLDivElement, ExpansionPanelContentProps> and just leave ExpansionPanelContent as a function definition.
Same applies to the other components like ExpansionPanel, ExpansionPanelToggle, and ExpansionPanelHeader
There was a problem hiding this comment.
Done in 20a0d64 — dropped React.forwardRef from all four subcomponents (plain function definitions).
Heads-up: CLAUDE.md currently lists React.forwardRef as the design-system convention (that's what the Doistbot review cited), so you may want to nuance that doc. Happy to follow your call on it here.
— Claude
| } from './expansion-panel' | ||
|
|
||
| export default { | ||
| title: '📐 Layout/ExpansionPanel', |
There was a problem hiding this comment.
Is this really a layout component? In https://github.com/Doist/frontend-issues/issues/1350 I've grouped this under "🧭 Navigation & structure", which we don't have in Reactist yet. Would you agree that's a more appropriate category?
There was a problem hiding this comment.
Done in 20a0d64 — moved to 🧭 Navigation & structure/ExpansionPanel. Agreed that's a better fit than Layout.
— Claude
| export const IconToggle = { | ||
| name: 'Icon toggle', | ||
| render: () => ( | ||
| <Box maxWidth="small"> |
There was a problem hiding this comment.
There was a problem hiding this comment.
Done in 20a0d64 — the demo panel now has a background (background="aside" + border radius + padding) so it reads as a distinct panel and the chevron no longer feels disconnected from the title.
— Claude
| alignItems="center" | ||
| justifyContent="spaceBetween" | ||
| > | ||
| <Text size="caption" weight="semibold" tone="secondary"> |
There was a problem hiding this comment.
Does it make sense that these are smaller than the content below? Caption = 12px and body is 14px.
There was a problem hiding this comment.
Yes, I aligned in e3737de: the title now uses size="body" (14px, matching the content) with tone="secondary" + weight="semibold", matching todoist-web's sidebar section header.
- Drop React.forwardRef from all subcomponents (plain functions); the DOM
elements aren't part of the documented API
- Remove the `as` casts on the toggle prop spread — plain `{...props}` type-checks
- Drive the animated container's overflow and display via Box props instead of a
className/style split (`overflow={...}`, `display={...}`); drop overflow from
`.entered`
- Rename HEIGHT_TRANSITION_DURATION -> HEIGHT_TRANSITION_DURATION_MS
- Story: move under "Navigation & structure", give the demo panel a background so
it reads as a panel, and size the title to match (not below) the content
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on header Use size=body, tone=secondary, weight=semibold to match the section header title styling in todoist-web. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
||
| export default { | ||
| title: '📐 Layout/ExpansionPanel', | ||
| title: '🧭 Navigation & structure/ExpansionPanel', |
There was a problem hiding this comment.
One thing we missed is that the Storybook hierarchy is specifically defined:
reactist/.storybook/preview.ts
Lines 22 to 38 in 53777ff
Without adding this new category there, it ends up at the bottom of the nav:
Can we insert it into the list in alphabetical order?


Overview
The aim is to introduce a new expansion panel component in Reactist so we can expand and collapse sidebar sections in Comms and later implement the same component in Todoist. Demo here.
This is related to the exploratory Expand Collapse PR here. Once we have shipped the Reactist component, I can go back and update the Comms sidebar PR to use the Reactist component.
Preview