Skip to content

Commit

Permalink
Accordion cleanup and release (#2119)
Browse files Browse the repository at this point in the history
## 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: #2119
  • Loading branch information
nishasy authored Nov 16, 2023
1 parent d8d6cf9 commit b13d432
Show file tree
Hide file tree
Showing 10 changed files with 59 additions and 47 deletions.
5 changes: 5 additions & 0 deletions .changeset/wet-apricots-wait.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-accordion": minor
---

Cleanup: prioritize all children overloaded props, fix typos, update documentation, make the accordion component public
18 changes: 9 additions & 9 deletions __docs__/wonder-blocks-accordion/accordion-section.argtypes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export default {
Defaults to "end".`,
defaultValue: "end",
table: {
category: "Visual style",
defaultValue: {summary: "end"},
type: {summary: '"start" | "end"'},
},
Expand All @@ -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"'},
},
Expand All @@ -73,6 +75,7 @@ export default {
be expanded. Defaults to true.`,
defaultValue: true,
table: {
category: "State",
defaultValue: {summary: "true"},
type: {summary: "boolean"},
},
Expand All @@ -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"},
},
Expand All @@ -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"},
},
Expand All @@ -100,6 +105,7 @@ export default {
control: {type: null},
description: "Called when the header is clicked.",
table: {
category: "State",
type: {summary: "(newExpandedState: boolean) => unknown"},
},
},
Expand All @@ -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},
Expand All @@ -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},
Expand All @@ -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},
Expand All @@ -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},
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,13 @@ export const Uncontrolled: StoryComponentType = {
render: function Render() {
return (
<AccordionSection
header="Controlled section"
header="Uncontrolled section"
onToggle={() =>
// 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
</AccordionSection>
);
},
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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 = {
Expand Down
6 changes: 6 additions & 0 deletions __docs__/wonder-blocks-accordion/accordion.argtypes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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"},
},
Expand All @@ -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"'},
},
Expand All @@ -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"'},
},
Expand All @@ -81,6 +85,7 @@ export default {
Defaults to false.`,
defaultValue: false,
table: {
category: "Visual style",
defaultValue: {summary: false},
type: {summary: "boolean"},
},
Expand All @@ -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},
Expand Down
6 changes: 3 additions & 3 deletions __docs__/wonder-blocks-accordion/accordion.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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).
Expand Down
3 changes: 1 addition & 2 deletions packages/wonder-blocks-accordion/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<AccordionSection header="Title" testId="accordion-section">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,14 +322,14 @@ describe("Accordion", () => {
});
});

test("prioritizes the parent's caretPosition prop", () => {
test("prioritizes the child's caretPosition prop", () => {
// Arrange
render(
<Accordion caretPosition="start">
<Accordion caretPosition="end">
{[
<AccordionSection
header="Title"
caretPosition="end"
caretPosition="start"
testId="section-test-id"
>
Section content
Expand All @@ -348,14 +348,14 @@ describe("Accordion", () => {
});
});

test("prioritizes the parent's animated prop", () => {
test("prioritizes the child's animated prop", () => {
// Arrange
render(
<Accordion animated={true}>
<Accordion animated={false}>
{[
<AccordionSection
header="Title"
animated={false}
animated={true}
testId="section-test-id"
>
Section content
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
};
}
}

Expand Down
20 changes: 10 additions & 10 deletions packages/wonder-blocks-accordion/src/components/accordion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -245,15 +245,15 @@ const Accordion = React.forwardRef(function Accordion(
// be list items.
<li key={index} id={id}>
{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),
Expand Down

0 comments on commit b13d432

Please sign in to comment.