feat!: Make Summary Box more flexible #1929

hgarfinkle · 2022-03-24T00:30:50Z

⚠️ Set the following as the commit body when merging this PR: BREAKING CHANGE: SummaryBox now exposes sub-components (SummaryBoxHeading and SummaryBoxContent) for a more compositional API. Consumers will need to update their implementation to match.

Summary

Makes the SummaryBox component more flexible, separating out SummaryBoxHeading and SummaryBoxHeading into their own components.
Is NOT backwards compatible.

Related Issues or PRs

Closes #1916

How To Test

Try the SummaryBox Storybook story to ensure it still displays properly

Screenshots (optional)

…react-uswds into ig-1916-flexible-summary-box

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.tsx

kimallen · 2022-03-28T18:35:07Z

I'm wondering if it would be worth breaking out the headingLevel change into its own issue/PR from the breaking change so that the headingLevel flexibility can be incorporated as a minor non-breaking change before the breaking change.

brandonlenz

@Igarfinkle thanks so much for picking this up!

A couple of suggestions and one question to some other maintainers in my review here.

One nit: We don't seem to have a unified pattern for how we name folders when double nesting things, (e.g. in this case SummaryBox/SummaryBox, or Footer/Footer or card/Card). JS, React, and TS are pretty unopinionated about this, so none are wrong.

Personally, if a folder does not directly contain a component file, I think it should be lower-cased (ie. summarybox/SummaryBox)

@kylehilltruss @suzubara @haworku thoughts? Some people prefer snakeCase or kebab-case for multi-word directories. If we come to a consensus, I'll draft a PR + ADR to set them all straight 😉

src/components/SummaryBox/SummaryBox/SummaryBox.test.tsx

brandonlenz · 2022-03-28T18:25:40Z

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.tsx

+import classnames from 'classnames'
+
+interface SummaryBoxHeadingProps {
+  heading: string


instead of a heading prop, can we use children here? That way the api can look like <SummaryBoxHeading>My Heading Content</SummaryBoxHeading>

That'll also allow more flexibility over just string in case someone wanted to otherwise stylize or pass in custom tags. (children should allow ReactNode as a type)

@Igarfinkle Did you decide to use heading prop anyway instead of children? I liked @brandonlenz 's suggestion

either way, a comment here to explain why it was resolved without changes would be helpful to future viewers.

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.tsx

src/components/SummaryBox/SummaryBox/SummaryBox.test.tsx

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.tsx

brandonlenz

Approving but with a couple of tiny things to fix listed below.

We'll likely want to time the release of this (since it's breaking), so lets work that out in our slack

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.test.tsx

src/components/SummaryBox/SummaryBox/SummaryBox.test.tsx

haworku

LGTM. I made #2000 to address the issue brandon brought up.

brandonlenz

Approving now since I think we've done all the communicating around an intentionally breaking release

brandonlenz · 2022-04-20T17:40:21Z

@Igarfinkle when merging this, please make sure that the commit body of the merge commit contains some verbiage like

BREAKING CHANGE: SummaryBox now exposes sub-components (SummaryBoxHeading and SummaryBoxContent) for a more compositional API. Consumers will need to update their implementation to match.

## [3.0.0](2.9.0...3.0.0) (2022-04-25) ### ⚠ BREAKING CHANGES * Previous deprecated features and props have been removed. Please see the following guidance for affected components: * Accordion: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop. * Alert: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop. * Button: * `accent` has been removed. Use `accentStyle` instead. * `big`, `small`, and `size="small"` have been removed. Use `size="big"` or do not define the `size` prop for default sizing. * CollectionHeading: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop. * Footer/Address: `big`, `medium`, and `slim` props have been removed. Use the `size` prop instead. * Footer/Footer: `big`, `medium`, and `slim` props have been removed. Use the `size` prop instead. * Footer/FooterNav: `big`, `medium`, and `small` props have been removed. Use the `size` prop instead. * Footer/Logo: `big`, `medium`, and `small` props have been removed. Use the `size` prop instead. * Search: `big`, and `small` props have been removed. Use the `size` prop instead. * Fieldset: `legendSrOnly` has been removed. Use `legendStyle="srOnly"` instead. * TextInput: `error`, and `success` props have been removed. Use the `validationStatus` prop instead. * header/NavList: `primary`, `secondary`, `subnav`, `megamenu`, and `footerSecondary` props have been removed. Use the `type` prop instead. * StepIndicator: Default heading level has been removed. Consumers must now specify via the `headingLevel` prop. * SummaryBox now exposes sub-components (SummaryBoxHeading and SummaryBoxContent) for a more compositional API. Consumers will need to update their implementation to match. * In order to accommodate IconList as a named component, Icons themselves needed to be refactored. All use of ReactUSWDS icons now follows the following syntax: <Icon.{IconName} /> instead of <Icon{IconName />. Furthermore, icons are no longer imported individually. Instead, Icon (the class) is imported to then use any <Icon.{IconName}> consumers require. ### Features * add isInitiallyOpen option to modal ([#1971](#1971)) ([560564e](560564e)) * Make Summary Box more flexible ([#1929](#1929)) ([a46ed35](a46ed35)) * New Component: IconList ([#1691](#1691)) ([86589ac](86589ac)) * **ci:** add automerge priority label for Kodiak ([#1985](#1985)) ([9dc940e](9dc940e)) * Remove 1.x.x and 2.x.x deprecated properties ([#1988](#1988)) ([5dfadb1](5dfadb1)) ### Documentation & Examples * 404 page template added ([#2017](#2017)) ([0c9474d](0c9474d))

Isaac Garfinkle added 8 commits March 23, 2022 16:34

Separate Summary Box Heading and text to their own components

4d36918

move files around

ae1847e

move content component to its own file

17273a4

add SummaryBoxContent tests

d8605c2

Move SummaryBoxHeading to its own file

8c6236b

add tests for SummaryBoxHeading

997e224

Change SummaryBox to not create Heading and Content automatically

0e914fe

fix tests and story

d61fd56

hgarfinkle requested review from suzubara, gidjin, haworku, ahobson, brandonlenz and SirenaBorracha as code owners March 24, 2022 00:30

hgarfinkle requested a review from a user March 24, 2022 00:30

Merge branch 'main' into ig-1916-flexible-summary-box

dd71d15

hgarfinkle changed the title ~~Make Summary Box more flexible~~ feat: Make Summary Box more flexible Mar 24, 2022

trussworks-infra-zz deployed to storybook March 24, 2022 00:32 View deployment

Isaac Garfinkle added 2 commits March 23, 2022 17:48

fix tests

519d26b

Merge branch 'ig-1916-flexible-summary-box' of github.com:trussworks/…

b8ca346

…react-uswds into ig-1916-flexible-summary-box

trussworks-infra-zz deployed to storybook March 24, 2022 01:11 View deployment

kimallen changed the title ~~feat: Make Summary Box more flexible~~ !feat: Make Summary Box more flexible Mar 24, 2022

kimallen reviewed Mar 24, 2022

View reviewed changes

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.tsx Show resolved Hide resolved

kimallen changed the title ~~!feat: Make Summary Box more flexible~~ feat!: Make Summary Box more flexible Mar 24, 2022

use default value for heading level

fbb49ba

trussworks-infra-zz deployed to storybook March 24, 2022 23:12 View deployment

brandonlenz reviewed Mar 28, 2022

View reviewed changes

brandonlenz added the type: breaking This introduces breaking changes and will require a major version increase label Mar 28, 2022

make SummaryBoxHeading take children instead of heading key

d584e02

trussworks-infra-zz deployed to storybook March 29, 2022 17:41 View deployment

remove default value for heading level

1fe4b8c

trussworks-infra-zz deployed to storybook March 29, 2022 17:43 View deployment

brandonlenz reviewed Mar 29, 2022

View reviewed changes

src/components/SummaryBox/SummaryBox/SummaryBox.test.tsx Show resolved Hide resolved

test for existence of heading

db42d75

trussworks-infra-zz deployed to storybook March 30, 2022 05:54 View deployment

fix tests

ad16926

trussworks-infra-zz deployed to storybook March 30, 2022 06:05 View deployment

brandonlenz reviewed Mar 30, 2022

View reviewed changes

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.tsx Outdated Show resolved Hide resolved

prefer getByRole

d08b10c

trussworks-infra-zz deployed to storybook March 30, 2022 18:09 View deployment

Merge branch 'main' into ig-1916-flexible-summary-box

e8f0097

trussworks-infra-zz deployed to storybook March 30, 2022 18:11 View deployment

brandonlenz previously approved these changes Mar 30, 2022

View reviewed changes

src/components/SummaryBox/SummaryBoxHeading/SummaryBoxHeading.test.tsx Outdated Show resolved Hide resolved

src/components/SummaryBox/SummaryBox/SummaryBox.test.tsx Outdated Show resolved Hide resolved

no unused vars

5fabc08

hgarfinkle dismissed brandonlenz’s stale review via 5fabc08 March 31, 2022 05:58

Merge branch 'main' into ig-1916-flexible-summary-box

b921032

trussworks-infra-zz deployed to storybook March 31, 2022 06:00 View deployment

haworku mentioned this pull request Apr 19, 2022

[chore] Unify directory naming #2000

Open

haworku approved these changes Apr 19, 2022

View reviewed changes

brandonlenz self-requested a review April 19, 2022 22:39

brandonlenz approved these changes Apr 19, 2022

View reviewed changes

Merge branch 'main' into ig-1916-flexible-summary-box

42795c2

hgarfinkle requested a review from rogeruiz as a code owner April 21, 2022 16:12

trussworks-infra-zz deployed to storybook April 21, 2022 16:13 View deployment

hgarfinkle merged commit a46ed35 into main Apr 21, 2022

hgarfinkle deleted the ig-1916-flexible-summary-box branch April 21, 2022 17:16

brandonlenz mentioned this pull request Apr 25, 2022

chore(release): 3.0.0 #2040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Make Summary Box more flexible #1929

feat!: Make Summary Box more flexible #1929

hgarfinkle commented Mar 24, 2022 •

edited by brandonlenz

Loading

kimallen commented Mar 28, 2022

brandonlenz left a comment

brandonlenz Mar 28, 2022 •

edited

Loading

kimallen Mar 30, 2022

kimallen Mar 30, 2022

brandonlenz left a comment

haworku left a comment

brandonlenz left a comment

brandonlenz commented Apr 20, 2022

feat!: Make Summary Box more flexible #1929

feat!: Make Summary Box more flexible #1929

Conversation

hgarfinkle commented Mar 24, 2022 • edited by brandonlenz Loading

⚠️ Set the following as the commit body when merging this PR: BREAKING CHANGE: SummaryBox now exposes sub-components (SummaryBoxHeading and SummaryBoxContent) for a more compositional API. Consumers will need to update their implementation to match.

Summary

Related Issues or PRs

How To Test

Screenshots (optional)

kimallen commented Mar 28, 2022

brandonlenz left a comment

Choose a reason for hiding this comment

brandonlenz Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

kimallen Mar 30, 2022

Choose a reason for hiding this comment

kimallen Mar 30, 2022

Choose a reason for hiding this comment

brandonlenz left a comment

Choose a reason for hiding this comment

haworku left a comment

Choose a reason for hiding this comment

brandonlenz left a comment

Choose a reason for hiding this comment

brandonlenz commented Apr 20, 2022

hgarfinkle commented Mar 24, 2022 •

edited by brandonlenz

Loading

⚠️ Set the following as the commit body when merging this PR: `BREAKING CHANGE: SummaryBox now exposes sub-components (SummaryBoxHeading and SummaryBoxContent) for a more compositional API. Consumers will need to update their implementation to match.`

brandonlenz Mar 28, 2022 •

edited

Loading