Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Update Accordion component default behavior to match USWDS #922

Merged
merged 13 commits into from
Mar 9, 2021
Merged

feat: Update Accordion component default behavior to match USWDS #922

merged 13 commits into from
Mar 9, 2021

Conversation

SirenaBorracha
Copy link
Contributor

@SirenaBorracha SirenaBorracha commented Feb 19, 2021 •
edited
Loading

Summary

This PR adds an optional multiselectable property to the Accordion component. In order to match the default behavior of the USWDS Accordion, the default value of the multiselectable property is false, so that only one item in the Accordion may be opened at a time. Optionally setting multiselectable to true allows for multiple items in the accordion to be expanded simultaneously.

The github issue identifies this as a "feat" but I'm wondering if it shouldn't be a "fix" ?

Related Issues or PRs

Github issue #207

How To Test

Checkout this branch and run yarn storybook

To see the intended default behavior

  • Navigate to Accordion > Borderless or Bordered
  • Toggle multiple items open and see that only one item may be expanded at a time, attempting to open a second item closes the previously opened item

To see the behavior when multiselectable is true

  • Navigate to Accordion > Multiselectable
  • Toggle multiple items open and see that you can expand and close items to your hearts content

Screenshots (optional)

Accordion with two items expanded
Screen Shot 2021-02-18 at 5 55 30 PM

Accordion with all items expanded
Screen Shot 2021-02-18 at 5 55 53 PM

@SirenaBorracha SirenaBorracha changed the title [WIP]feat: Update Accordion component default behavior to match USWDS feat: Update Accordion component default behavior to match USWDS Feb 19, 2021
ahobson
ahobson reviewed Feb 19, 2021
View reviewed changes
src/components/Accordion/Accordion.tsx
},
className
)

const toggleItem = (itemId: AccordionItem['id']): void => {
const newOpenItems = [...openItems]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sooo ... it turns out that const in TypeScript just means you cannot reassign the variable. You can still invoke methods that change the object.

Here's a playground that shows adding an element to a const array.

brandonlenz
brandonlenz reviewed Feb 19, 2021
View reviewed changes
Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good! I have a few small suggestions 🎉

src/components/Accordion/Accordion.test.tsx Outdated Show resolved Hide resolved
src/components/Accordion/Accordion.test.tsx Outdated Show resolved Hide resolved
src/components/Accordion/Accordion.tsx Outdated Show resolved Hide resolved
@suzubara suzubara added this to the v2 milestone Feb 19, 2021
@SirenaBorracha SirenaBorracha changed the base branch from main to V2 February 19, 2021 23:06
@SirenaBorracha SirenaBorracha marked this pull request as ready for review February 19, 2021 23:55
@SirenaBorracha SirenaBorracha linked an issue Feb 19, 2021 that may be closed by this pull request
[feat] Accordion component should support single selectable section behavior by default #207
Closed
suzubara
suzubara reviewed Feb 20, 2021
View reviewed changes
src/components/Accordion/Accordion.tsx Outdated
@@ -61,20 +62,26 @@ export const Accordion = (props: AccordionProps): React.ReactElement => {
'usa-accordion',
{
'usa-accordion--bordered': bordered,
'aria-multiselectable': multiselectable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is supposed to be a CSS class! instead, the Accordion <div> should have the aria-multiselectable attribute if multiselectable is true (I'm comparing with the source code at https://designsystem.digital.gov/components/accordion/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thank you for catching that.

@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook March 8, 2021 20:41 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook March 8, 2021 23:06 Inactive
@haworku
Copy link
Contributor

haworku commented Mar 9, 2021

@SirenaBorracha LGTM.

I didn't approve bc right now it looks like V2 branch is not up to date so this PR looks like its going to merge a lot more behavior than it does. Can you work with @brandonlenz to bring V2 up to date before you merge? I think he might be working on something with this. Should just be a matter of fastforwarding v2 up to date. Otherwise I think this work looks ready.

brandonlenz
brandonlenz requested changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small thing, then I'm good to approve!

src/components/Accordion/Accordion.tsx Outdated Show resolved Hide resolved
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook March 9, 2021 20:11 Inactive
@brandonlenz brandonlenz self-requested a review March 9, 2021 20:35
brandonlenz
brandonlenz approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of that! Approved 👍

@SirenaBorracha SirenaBorracha merged commit de6d1c7 into V2 Mar 9, 2021
@SirenaBorracha SirenaBorracha deleted the ak-update-accordion-component-default-behavior-207 branch March 9, 2021 22:21
@suzubara suzubara added the type: breaking This introduces breaking changes and will require a major version increase label Apr 12, 2021
@suzubara suzubara removed this from the v2 milestone Apr 12, 2021
brandonlenz pushed a commit that referenced this pull request May 12, 2021
@SirenaBorracha @brandonlenz
feat!: Update Accordion component default behavior to match USWDS (#922)
a89634f 
BREAKING CHANGE: If the multiselectable behavior is desired, the multiselectable prop must now be passed to the Accordion component
brandonlenz pushed a commit that referenced this pull request May 12, 2021
@SirenaBorracha @brandonlenz
feat!: Update Accordion component default behavior to match USWDS (#922)
c063047 
BREAKING CHANGE: To continue to use the multiselectable behavior, use the multiselectable prop
@haworku haworku mentioned this pull request Jun 14, 2021
Merged
@brandonlenz brandonlenz mentioned this pull request Jun 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahobson ahobson ahobson left review comments

@suzubara suzubara suzubara left review comments

@brandonlenz brandonlenz brandonlenz approved these changes

@haworku haworku Awaiting requested review from haworku

@gidjin gidjin Awaiting requested review from gidjin

@sarboc sarboc Awaiting requested review from sarboc

@tinyels tinyels Awaiting requested review from tinyels

Assignees
No one assigned
Labels
type: breaking This introduces breaking changes and will require a major version increase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feat] Accordion component should support single selectable section behavior by default
6 participants
@SirenaBorracha @haworku @ahobson @suzubara @brandonlenz @trussworks-infra-zz