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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
78f928b
Commenting where multiselectable will go
SirenaBorracha Feb 9, 2021
3e0d901
Merge branch 'main' into ak-update-accordion-component-default-behavi…
SirenaBorracha Feb 11, 2021
80f9e23
Committing comments so I can merge changes from main
SirenaBorracha Feb 18, 2021
9d0b983
Merge branch 'main' into ak-update-accordion-component-default-behavi…
SirenaBorracha Feb 18, 2021
df78225
Setting multiselectable prop to false as default, changed const to le…
SirenaBorracha Feb 18, 2021
896f5e3
Add multiselectable example to Storybook
SirenaBorracha Feb 19, 2021
9178f2a
Adding test checking for ability to open (and close) multiple differe…
SirenaBorracha Feb 19, 2021
f635e73
Amending test language to better represent what the test is testing, …
SirenaBorracha Feb 19, 2021
4ca693d
Adding test explicitly testing default behavior where multiselectable…
SirenaBorracha Feb 19, 2021
a9f851a
Merge branch 'main' into ak-update-accordion-component-default-behavi…
SirenaBorracha Feb 19, 2021
ae21859
Merge branch 'main' into ak-update-accordion-component-default-behavi…
SirenaBorracha Mar 8, 2021
ca87b19
Make multiselectable an attribute passed to the Accordion's parent di…
SirenaBorracha Mar 8, 2021
dc8c411
Add logic so aria-multiselectable is not set if multiselectable is fa…
SirenaBorracha Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/components/Accordion/Accordion.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ export const bordered = (): React.ReactElement => (
<Accordion bordered={true} items={testItems} />
)

export const multiselectable = (): React.ReactElement => (
<Accordion items={testItems} multiselectable={true} />
)

const customTestItems = [
{
title: (
Expand Down
71 changes: 71 additions & 0 deletions src/components/Accordion/Accordion.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,77 @@ describe('Accordion component', () => {
})
})

describe('when multiselectable is false (default behavior)', () => {
it('when an item is opened, clicking a different item closes the previously opened item', () => {
const { getByText, getByTestId } = render(<Accordion items={testItems} />)

expect(getByTestId(`accordionItem_${testItems[0].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[1].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[2].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[3].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[4].id}`)).not.toBeVisible()

fireEvent.click(getByText(testItems[3].title))
fireEvent.click(getByText(testItems[1].title))

expect(getByTestId(`accordionItem_${testItems[0].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[1].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[2].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[3].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[4].id}`)).not.toBeVisible()

fireEvent.click(getByText(testItems[4].title))
fireEvent.click(getByText(testItems[2].title))

expect(getByTestId(`accordionItem_${testItems[0].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[1].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[2].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[3].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[4].id}`)).not.toBeVisible()
})
})

describe('when multiselectable is true', () => {
it('when an item is opened, previously open items remain open', () => {
const { getByText, getByTestId } = render(
<Accordion items={testItems} multiselectable={true} />
)

expect(getByTestId(`accordionItem_${testItems[0].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[1].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[2].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[3].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[4].id}`)).not.toBeVisible()

fireEvent.click(getByText(testItems[0].title))
fireEvent.click(getByText(testItems[1].title))

expect(getByTestId(`accordionItem_${testItems[0].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[1].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[2].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[3].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[4].id}`)).not.toBeVisible()

fireEvent.click(getByText(testItems[0].title))
fireEvent.click(getByText(testItems[3].title))

expect(getByTestId(`accordionItem_${testItems[0].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[1].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[2].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[3].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[4].id}`)).not.toBeVisible()

fireEvent.click(getByText(testItems[2].title))
fireEvent.click(getByText(testItems[4].title))

expect(getByTestId(`accordionItem_${testItems[0].id}`)).not.toBeVisible()
expect(getByTestId(`accordionItem_${testItems[1].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[2].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[3].id}`)).toBeVisible()
expect(getByTestId(`accordionItem_${testItems[4].id}`)).toBeVisible()
})
})

describe('with expanded items on mount', () => {
const testExpandedItems = [
{
Expand Down
17 changes: 13 additions & 4 deletions src/components/Accordion/Accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ interface AccordionItem {

interface AccordionProps {
bordered?: boolean
multiselectable?: boolean
items: AccordionItem[]
className?: string
}
Expand Down Expand Up @@ -51,7 +52,7 @@ export const AccordionItem = (props: AccordionItem): React.ReactElement => {
}

export const Accordion = (props: AccordionProps): React.ReactElement => {
const { bordered, items, className } = props
const { bordered, items, className, multiselectable = false } = props

const [openItems, setOpenState] = useState(
items.filter((i) => !!i.expanded).map((i) => i.id)
Expand All @@ -68,18 +69,26 @@ export const Accordion = (props: AccordionProps): React.ReactElement => {
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.

const itemIndex = openItems.indexOf(itemId)
const isMultiselectable = multiselectable

if (itemIndex > -1) {
newOpenItems.splice(itemIndex, 1)
} else {
newOpenItems.push(itemId)
if (isMultiselectable) {
newOpenItems.push(itemId)
} else {
newOpenItems.splice(0, newOpenItems.length)
newOpenItems.push(itemId)
}
}

setOpenState(newOpenItems)
}

return (
<div className={classes} data-testid="accordion">
<div
className={classes}
data-testid="accordion"
aria-multiselectable={multiselectable}>
brandonlenz marked this conversation as resolved.
Show resolved Hide resolved
{items.map((item, i) => (
<AccordionItem
key={`accordionItem_${i}`}
Expand Down