From b13d4321b59eb1c8ad60a78f90be0c9a15fa3c1f Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Thu, 16 Nov 2023 09:33:34 -0800 Subject: [PATCH] Accordion cleanup and release (#2119) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: All that's left is some polishing, and the Accordion component should be ready for launch! In this PR: - Prioritize children's values for overloaded props - Fix the issue where the bottom border radius was applying to non-last section content when it wasn't supposed to - Update the Uncontrolled AccordionSection story so it actually says it's uncontrolled - Add categories to prop tables - Fixed typo - Update comments and documentation accordingly - Make Accordion package.json say that the package is public! 🎉 Issue: https://khanacademy.atlassian.net/browse/WB-1585 ## Test plan: `yarn jest` Storybook - Confirm that Accordion's prop table has the props split up into sections now - Confirm that AccordionSection's prop table has the props split the sections now - Confirm that all the documentation says that the children override the parents' props - Go to Accordion's "React Element in Children" story and confirm that the corners don't have overflow. Author: nishasy Reviewers: jandrade Required Reviewers: Approved By: jandrade Checks: ✅ codecov/project, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 16.x), ✅ Lint (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 2/2), ✅ Check build sizes (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 1/2), ✅ Test (ubuntu-latest, 16.x, 2/2), ✅ Check build sizes (ubuntu-latest, 16.x), ✅ Lint (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 1/2), ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ✅ gerald, ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 16.x), ⏭ Publish npm snapshot, ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ⏭ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 16.x), ✅ gerald, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 16.x), ✅ Lint (ubuntu-latest, 16.x), ✅ Check build sizes (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 2/2), ✅ Test (ubuntu-latest, 16.x, 1/2), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 16.x), ⏭ Publish npm snapshot, ⏭ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 16.x), ✅ gerald Pull Request URL: https://github.com/Khan/wonder-blocks/pull/2119 --- .changeset/wet-apricots-wait.md | 5 ++++ .../accordion-section.argtypes.tsx | 18 +++++++------- .../accordion-section.stories.tsx | 8 +++---- .../accordion.argtypes.tsx | 6 +++++ .../accordion.stories.tsx | 6 ++--- packages/wonder-blocks-accordion/package.json | 3 +-- .../__tests__/accordion-section.test.tsx | 2 +- .../components/__tests__/accordion.test.tsx | 14 +++++------ .../src/components/accordion-section.tsx | 24 ++++++++++--------- .../src/components/accordion.tsx | 20 ++++++++-------- 10 files changed, 59 insertions(+), 47 deletions(-) create mode 100644 .changeset/wet-apricots-wait.md diff --git a/.changeset/wet-apricots-wait.md b/.changeset/wet-apricots-wait.md new file mode 100644 index 000000000..72eff1b84 --- /dev/null +++ b/.changeset/wet-apricots-wait.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/wonder-blocks-accordion": minor +--- + +Cleanup: prioritize all children overloaded props, fix typos, update documentation, make the accordion component public diff --git a/__docs__/wonder-blocks-accordion/accordion-section.argtypes.tsx b/__docs__/wonder-blocks-accordion/accordion-section.argtypes.tsx index e3ad026f5..e4b832b1f 100644 --- a/__docs__/wonder-blocks-accordion/accordion-section.argtypes.tsx +++ b/__docs__/wonder-blocks-accordion/accordion-section.argtypes.tsx @@ -39,6 +39,7 @@ export default { Defaults to "end".`, defaultValue: "end", table: { + category: "Visual style", defaultValue: {summary: "end"}, type: {summary: '"start" | "end"'}, }, @@ -57,6 +58,7 @@ export default { there is white space between each section.`, defaultValue: "rounded", table: { + category: "Visual style", defaultValue: {summary: "rounded"}, type: {summary: '"square" | "rounded" | "rounded-per-section"'}, }, @@ -73,6 +75,7 @@ export default { be expanded. Defaults to true.`, defaultValue: true, table: { + category: "State", defaultValue: {summary: "true"}, type: {summary: "boolean"}, }, @@ -82,6 +85,7 @@ export default { description: `Whether this section should be expanded on initial load. Defaults to false.`, table: { + category: "State", defaultValue: {summary: "false"}, type: {summary: "boolean"}, }, @@ -92,6 +96,7 @@ export default { be false if the user has \`prefers-reduced-motion\` opted in. Defaults to false.`, table: { + category: "Visual style", defaultValue: {summary: "false"}, type: {summary: "boolean"}, }, @@ -100,6 +105,7 @@ export default { control: {type: null}, description: "Called when the header is clicked.", table: { + category: "State", type: {summary: "(newExpandedState: boolean) => unknown"}, }, }, @@ -108,6 +114,7 @@ export default { description: "Custom styles for the overall accordion section container.", table: { + category: "Visual style", type: {summary: "StyleType"}, }, type: {name: "StyleType", required: false}, @@ -116,6 +123,7 @@ export default { control: {type: "object"}, description: "Custom styles for the header.", table: { + category: "Visual style", type: {summary: "StyleType"}, }, type: {name: "StyleType", required: false}, @@ -126,6 +134,7 @@ export default { "h2", etc.) Please use this to ensure that the header is hierarchically correct.`, table: { + category: "Accessibility", type: {summary: "string"}, }, type: {name: "string", required: false}, @@ -139,13 +148,4 @@ export default { }, type: {name: "string", required: false}, }, - headerTestId: { - control: {type: "text"}, - description: `The test ID used to locate this component's - clickable header in automated tests.`, - table: { - type: {summary: "string"}, - }, - type: {name: "string", required: false}, - }, }; diff --git a/__docs__/wonder-blocks-accordion/accordion-section.stories.tsx b/__docs__/wonder-blocks-accordion/accordion-section.stories.tsx index 450ba6c54..f403257b3 100644 --- a/__docs__/wonder-blocks-accordion/accordion-section.stories.tsx +++ b/__docs__/wonder-blocks-accordion/accordion-section.stories.tsx @@ -172,13 +172,13 @@ export const Uncontrolled: StoryComponentType = { render: function Render() { return ( // eslint-disable-next-line no-console console.log("Click! This function is being called!") } > - This is the information present in this controlled section + This is the information present in this uncontrolled section ); }, @@ -272,7 +272,7 @@ export const ReactElementInChildren: StoryComponentType = { * * If the `caretPosition` prop is specified both here in the * AccordionSection and within the parent Accordion component, - * the Accordion's `caretPosition` value is prioritized. + * the AccordionSection's `caretPosition` value is prioritized. */ export const CaretPositions: StoryComponentType = { render: function Render() { @@ -451,7 +451,7 @@ export const NotCollapsible: StoryComponentType = { * the AccordionSection. * * If `animated` is specified both here in the AccordionSection - * and within a parent Accordion component, the Accordion's + * and within a parent Accordion component, the AccordionSection's * `animated` value is prioritized. */ export const WithAnimation: StoryComponentType = { diff --git a/__docs__/wonder-blocks-accordion/accordion.argtypes.tsx b/__docs__/wonder-blocks-accordion/accordion.argtypes.tsx index 70c503973..726e85a9a 100644 --- a/__docs__/wonder-blocks-accordion/accordion.argtypes.tsx +++ b/__docs__/wonder-blocks-accordion/accordion.argtypes.tsx @@ -25,6 +25,7 @@ export default { AccordionSections will be expanded when the Accordion is first rendered.`, table: { + category: "State", type: {summary: "number"}, }, type: {name: "number", required: false}, @@ -35,6 +36,7 @@ export default { same time. If not specified, multiple AccordionSections can be expanded at a time.`, table: { + category: "State", defaultValue: {summary: true}, type: {summary: "boolean"}, }, @@ -47,6 +49,7 @@ export default { languages) of the header block in this section. Defaults to "end".`, defaultValue: "end", table: { + category: "Visual style", defaultValue: {summary: "end"}, type: {summary: '"start" | "end"'}, }, @@ -65,6 +68,7 @@ export default { there is white space between each section.`, defaultValue: "rounded", table: { + category: "Visual style", defaultValue: {summary: "rounded"}, type: {summary: '"square" | "rounded" | "rounded-per-section"'}, }, @@ -81,6 +85,7 @@ export default { Defaults to false.`, defaultValue: false, table: { + category: "Visual style", defaultValue: {summary: false}, type: {summary: "boolean"}, }, @@ -90,6 +95,7 @@ export default { control: {type: "object"}, description: "Custom styles for the overall accordion container.", table: { + category: "Visual style", type: {summary: "StyleType"}, }, type: {name: "StyleType", required: false}, diff --git a/__docs__/wonder-blocks-accordion/accordion.stories.tsx b/__docs__/wonder-blocks-accordion/accordion.stories.tsx index 03a2385a3..b1c72fced 100644 --- a/__docs__/wonder-blocks-accordion/accordion.stories.tsx +++ b/__docs__/wonder-blocks-accordion/accordion.stories.tsx @@ -148,7 +148,7 @@ AllowMultipleExpanded.parameters = { * a left-to-right language (and on the left of a right-to-left language). * * If the `caretPosition` prop is specified both here in the Accordion and - * within a child AccordionSection component, the Accordion's + * within a child AccordionSection component, the AccordionSection's * `caretPosition` value is prioritized. */ export const CaretPositions: StoryComponentType = { @@ -289,7 +289,7 @@ export const WithInitialExpandedIndex: StoryComponentType = { * the Accordion. * * If `animated` is specified both here in the Accordion - * and within a child AccordionSection component, the Accordion's + * and within a child AccordionSection component, the AccordionSection's * `animated` value is prioritized. */ export const WithAnimation: StoryComponentType = { @@ -414,7 +414,7 @@ WithAnimation.parameters = { /** * An Accordion with custom styles. The custom styles in this example * include a pink border and extra padding. - * Note that the Accordian's border is different than the AccordionSection + * Note that the Accordion's border is different than the AccordionSection * border styles. Passing custom styles here will not affect the sections' * styles. If you want to change the corner kind of a single section, * that can be done using the `cornerKind` prop (as demonstrated here). diff --git a/packages/wonder-blocks-accordion/package.json b/packages/wonder-blocks-accordion/package.json index 8386e99c5..063fb23ff 100644 --- a/packages/wonder-blocks-accordion/package.json +++ b/packages/wonder-blocks-accordion/package.json @@ -12,9 +12,8 @@ "types": "dist/index.d.ts", "author": "", "license": "MIT", - "private": true, "publishConfig": { - "access": "restricted" + "access": "public" }, "dependencies": { "@khanacademy/wonder-blocks-clickable": "^4.0.11", diff --git a/packages/wonder-blocks-accordion/src/components/__tests__/accordion-section.test.tsx b/packages/wonder-blocks-accordion/src/components/__tests__/accordion-section.test.tsx index be483e282..32798efe2 100644 --- a/packages/wonder-blocks-accordion/src/components/__tests__/accordion-section.test.tsx +++ b/packages/wonder-blocks-accordion/src/components/__tests__/accordion-section.test.tsx @@ -172,7 +172,7 @@ describe("AccordionSection", () => { expect(header).toBeVisible(); }); - test("uses headerTestId as button's data-test-id", () => { + test("uses the header's testId as button's data-test-id", () => { // Arrange render( diff --git a/packages/wonder-blocks-accordion/src/components/__tests__/accordion.test.tsx b/packages/wonder-blocks-accordion/src/components/__tests__/accordion.test.tsx index a154ed10e..3ebb527d3 100644 --- a/packages/wonder-blocks-accordion/src/components/__tests__/accordion.test.tsx +++ b/packages/wonder-blocks-accordion/src/components/__tests__/accordion.test.tsx @@ -322,14 +322,14 @@ describe("Accordion", () => { }); }); - test("prioritizes the parent's caretPosition prop", () => { + test("prioritizes the child's caretPosition prop", () => { // Arrange render( - + {[ Section content @@ -348,14 +348,14 @@ describe("Accordion", () => { }); }); - test("prioritizes the parent's animated prop", () => { + test("prioritizes the child's animated prop", () => { // Arrange render( - + {[ Section content @@ -369,7 +369,7 @@ describe("Accordion", () => { const sectionHeader = screen.getByTestId("section-test-id-header"); // Assert - // The parent has animated=true, so the child's animated=false + // The child has animated=true, so the parent's animated=false // should be overridden. expect(sectionHeader).toHaveStyle({ // The existence of the transition style means that the diff --git a/packages/wonder-blocks-accordion/src/components/accordion-section.tsx b/packages/wonder-blocks-accordion/src/components/accordion-section.tsx index 13606c0dc..dc4c3ac28 100644 --- a/packages/wonder-blocks-accordion/src/components/accordion-section.tsx +++ b/packages/wonder-blocks-accordion/src/components/accordion-section.tsx @@ -37,7 +37,7 @@ type Props = AriaProps & { * Defaults to "end". * * If this prop is specified both here in the AccordionSection and - * within a parent Accordion component, the Accordion’s caretPosition + * within a parent Accordion component, the AccordionSection’s caretPosition * value is prioritized. */ caretPosition?: "start" | "end"; @@ -71,7 +71,7 @@ type Props = AriaProps & { * if the user has `prefers-reduced-motion` opted in. Defaults to false. * * If this prop is specified both here in the AccordionSection and - * within a parent Accordion component, the Accordion’s animated + * within a parent Accordion component, the AccordionSection’s animated * value is prioritized. */ animated?: boolean; @@ -379,27 +379,29 @@ const _generateStyles = ( borderBottom: "none", }; - contentWrapperStyle = { - // Give the content wrapper the same border radius as the wrapper - // so that the content doesn't overflow out the corners. We - // can't put `overflow: "hidden"` on the overall container - // because it cuts off the header's focus outline. - borderEndEndRadius: tokens.spacing.small_12, - borderEndStartRadius: tokens.spacing.small_12, - }; - if (isFirstSection) { firstSectionStyle = { borderStartStartRadius: tokens.spacing.small_12, borderStartEndRadius: tokens.spacing.small_12, }; } + if (isLastSection) { lastSectionStyle = { borderBottom: `1px solid ${tokens.color.offBlack16}`, borderEndStartRadius: tokens.spacing.small_12, borderEndEndRadius: tokens.spacing.small_12, }; + + contentWrapperStyle = { + // Give the last section's content wrapper the same bottom + // border radius as the wrapper so that the content doesn't + // overflow out the corners. This issue can't be solved by + // putting `overflow: "hidden"` on the overall container + // because that cuts off the header's focus outline. + borderEndEndRadius: tokens.spacing.small_12, + borderEndStartRadius: tokens.spacing.small_12, + }; } } diff --git a/packages/wonder-blocks-accordion/src/components/accordion.tsx b/packages/wonder-blocks-accordion/src/components/accordion.tsx index 0bbdf32ee..3c199ae63 100644 --- a/packages/wonder-blocks-accordion/src/components/accordion.tsx +++ b/packages/wonder-blocks-accordion/src/components/accordion.tsx @@ -44,7 +44,7 @@ type Props = AriaProps & { * Defaults to "end". * * If this prop is specified both here in the Accordion and within - * a child AccordionSection component, the Accordion’s caretPosition + * a child AccordionSection component, the AccordionSection’s caretPosition * value is prioritized. */ caretPosition?: "start" | "end"; @@ -65,7 +65,7 @@ type Props = AriaProps & { * if the user has `prefers-reduced-motion` opted in. Defaults to false. * * If this prop is specified both here in the Accordion and within - * a child AccordionSection component, the Accordion’s animated + * a child AccordionSection component, the AccordionSection’s animated * value is prioritized. */ animated?: boolean; @@ -228,7 +228,7 @@ const Accordion = React.forwardRef(function Accordion( caretPosition: childCaretPosition, cornerKind: childCornerKind, onToggle: childOnToggle, - animated: childanimated, + animated: childAnimated, } = child.props; // Create a ref for each child AccordionSection to @@ -245,15 +245,15 @@ const Accordion = React.forwardRef(function Accordion( // be list items.
  • {React.cloneElement(child, { - // Prioritize the Accordion's caretPosition - caretPosition: caretPosition ?? childCaretPosition, - // Prioritize the AccordionSection's cornerKind + // Prioritize AccordionSection's props when + // they're overloading Accordion's props. + animated: childAnimated ?? animated, + caretPosition: childCaretPosition ?? caretPosition, cornerKind: childCornerKind ?? cornerKind, - // Don't use the AccordionSection's expanded prop - // when it's rendered within Accordion. + // AccordionSection's expanded prop does not get + // used here when it's rendered within Accordion + // since the expanded state is managed by Accordion. expanded: sectionsOpened[index], - // Prioritize the Accordion's animated - animated: animated ?? childanimated, onToggle: () => handleSectionClick(index, childOnToggle), onFocus: () => handleSectionFocus(index),