Add toggleMode to accordion and accordion section #2559
Conversation
🦋 Changeset detectedLatest commit: b5a04ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +218 B (+0.22%) Total Size: 100 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-mluqphpong.chromatic.com/ Chromatic results:
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (e9e7c62) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2559"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2559 |
| key={`nested-section-1`} | ||
| caretPosition="start" | ||
| > | ||
| <AccordionSection |
There was a problem hiding this comment.
Can you add a small margin around the inner nested sections so it's visually more clear that they're nested?
| key={`nested-section-1`} | ||
| caretPosition="start" | ||
| > | ||
| <AccordionSection |
There was a problem hiding this comment.
suggestion: Looking at the rendered markup, it would be helpful to have Accordions output in a nested UL and LIs. That would indicate the structure semantically. Is there a way to handle this under the hood and make it automatic for consumers?
Design-wise, the nested accordions aren't super clear that they're nested beyond the caret icons and a subtle outline. You. might already be working on this, but can we make the nesting more obvious somehow? Some design ideas could include indentation or other spacing, or background/foreground colors.
| </View> | ||
|
|
||
| <View> | ||
| <LabelLarge>Nested Accordion Example</LabelLarge> |
There was a problem hiding this comment.
suggestion: Even though this uses toggle mode, could the nested example be broken out into a separate story? It would make it easier to find!
| expect(await screen.findByText("Content 1")).toBeVisible(); | ||
| expect(await screen.findByText("Content 2")).toBeVisible(); | ||
|
|
||
| // Act: now re-render with toggleMode="collapse-all" |
There was a problem hiding this comment.
question: Can you break this out into a separate test to isolate the assertions? Since the first set of expectations are testing the same visibility, perhaps they could also be combined into one group with a different query selector, like findAllByRole?
There are some tips and reasoning in this doc: https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/98402353/Unit+Testing+Best+Practices#Assert-One-Requirement-Per-Test
| @@ -73,6 +83,26 @@ type Props = AriaProps & { | |||
| * Custom styles for the overall accordion container. | |||
| */ | |||
| style?: StyleType; | |||
|
|
|||
| /** | |||
| * Called whenever the accordion’s overall expansion state may have changed, | |||
There was a problem hiding this comment.
praise: Thanks for these comments! They are super helpful!
| @@ -17,7 +17,7 @@ type Props = { | |||
| // Unique ID for this section's button. | |||
| id: string; | |||
| // Header content. | |||
| header: string | React.ReactElement; | |||
| header: string | React.ReactElement | TemplateStringsArray; | |||
There was a problem hiding this comment.
question: I'm curious, are there specific cases where TemplateStringsArray becomes necessary?
| }); | ||
|
|
||
| export const WithToggleMode: StoryComponentType = { |
There was a problem hiding this comment.
suggestion: Could you please add a JSdoc here to add a description to this Story? This would be helpful to devs that want to understand how to use this variant.
https://storybook.js.org/docs/api/doc-blocks/doc-block-description#writing-descriptions
| const [toggleMode, setToggleMode] = | ||
| React.useState<AccordionToggleMode>("collapse-all"); | ||
|
|
||
| const [toggleModeForNextedSection, setToggleModeForNextedSection] = |
There was a problem hiding this comment.
nit(typo):
| const [toggleModeForNextedSection, setToggleModeForNextedSection] = | |
| const [toggleModeForNestedSection, setToggleModeForNestedSection] = |
| @@ -17,7 +17,7 @@ type Props = { | |||
| // Unique ID for this section's button. | |||
| id: string; | |||
| // Header content. | |||
| header: string | React.ReactElement; | |||
| header: string | React.ReactElement | TemplateStringsArray; | |||
| onToggleModeComplete?: (status: { | ||
| anyExpanded: boolean; | ||
| anyCollapsed: boolean; | ||
| }) => void; |
There was a problem hiding this comment.
question: Is there a specific use case for this callback? By looking at the Story, I don't follow how this could be used in an instance.
| // Detect partial vs all open vs all closed sections | ||
| const anyOpen = newSectionsOpened.some(Boolean); | ||
| const anyClosed = newSectionsOpened.some((sectionOpen) => !sectionOpen); | ||
|
|
||
| if (anyOpen && anyClosed) { | ||
| onToggleModeComplete?.({anyExpanded: true, anyCollapsed: true}); | ||
| } else { | ||
| onToggleModeComplete?.({ | ||
| anyExpanded: anyOpen, | ||
| anyCollapsed: anyClosed, | ||
| }); | ||
| } |
There was a problem hiding this comment.
suggestion: If this callback still needs to be exposed to the consumer, I think it would be best from the API perspective to return the same value of toggleMode. That way we could let the consumer know why kind of toggle is currently used.
| // Detect partial vs all open vs all closed sections | |
| const anyOpen = newSectionsOpened.some(Boolean); | |
| const anyClosed = newSectionsOpened.some((sectionOpen) => !sectionOpen); | |
| if (anyOpen && anyClosed) { | |
| onToggleModeComplete?.({anyExpanded: true, anyCollapsed: true}); | |
| } else { | |
| onToggleModeComplete?.({ | |
| anyExpanded: anyOpen, | |
| anyCollapsed: anyClosed, | |
| }); | |
| } | |
| // Detect partial vs all open vs all closed sections | |
| const currentToggleMode = anyOpen && anyClosed ? internalToggleMode : "none"; | |
| onToggleModeComplete?.(currentToggleMode); |
By using this approach, it is easier for consumers to understand only a single type of input/output. instead of having to understand one input (union type) and a different output (object with booleans).
| * `expand-all` opens them all. | ||
| * `collapse-all` closes them all. | ||
| * `none` leaves their state user-controlled. |
There was a problem hiding this comment.
suggestion: This is great! just an extra - would help formatting the docs even more :)
| * `expand-all` opens them all. | |
| * `collapse-all` closes them all. | |
| * `none` leaves their state user-controlled. | |
| * - `expand-all` opens them all. | |
| * - `collapse-all` closes them all. | |
| * - `none` leaves their state user-controlled. |
| | undefined; | ||
| type Props = AriaProps & { |
There was a problem hiding this comment.
nit: it would be nice adding a line break (suuper nit 😄)
| | undefined; | |
| type Props = AriaProps & { | |
| | undefined; | |
| type Props = AriaProps & { |
| if (toggleMode === "expand-all") { | ||
| setSectionsOpened(Array(children.length).fill(true)); | ||
| } else if (toggleMode === "collapse-all") { | ||
| setSectionsOpened(Array(children.length).fill(false)); | ||
| } |
There was a problem hiding this comment.
suggestion: Thanks for adding the comment above! I think this could be simplified to:
| if (toggleMode === "expand-all") { | |
| setSectionsOpened(Array(children.length).fill(true)); | |
| } else if (toggleMode === "collapse-all") { | |
| setSectionsOpened(Array(children.length).fill(false)); | |
| } | |
| if (toggleMode !== "none") { | |
| setSectionsOpened(Array(children.length).fill(toggleMode === "expand-all")); | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2559 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary:
This PR updates the Accordion and AccordionSection so that sections can be nested and exposes an internal toggle handler.
Updates:
Screenshots:
Screen.Recording.2025-04-15.at.9.50.50.PM.mov
Issue: CLASS-11631
Test plan: