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

Update defaults of allowMultipleExpanded and allowZeroExpanded to true #366

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Update defaults of allowMultipleExpanded and allowZeroExpanded to true #366

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f98b233
Update defaults of allowMultipleExpanded and allowZeroExpanded to true
liamjohnston Aug 22, 2023
a6e45d1
Update src/components/Accordion.spec.tsx
liamjohnston Aug 23, 2023
44e2c1d
- Revert intergration test case to use defaults (noting the defaults …
liamjohnston Aug 23, 2023
cedb6b0
Merge branch 'feature/change-default-settings' of https://github.com/…
liamjohnston Aug 23, 2023
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
17 changes: 17 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,23 @@
> All notable changes to this project are documented in this file. This project
> adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [[v6.0.0]](https://github.com/springload/react-accessible-accordion/releases/tag/v6.0.0)

**A sort-of breaking change** which updates the default behavior regarding
`allowMultipleExpanded` and `allowZeroExpanded`. They both now default to
`true`.

This is based on
[advice from NNG](https://www.nngroup.com/articles/accordions-on-desktop/), as
well as Springload's views on accordion usability.

From this version onwards:

- to disable allowing more than one accordion panel to be open at once, you
must explicitly set `allowMultipleExpanded={false}`
- to disable collapsing all accordion panels, you must explicitly set
`allowZeroExpanded={false}`

## [[v5.0.0]](https://github.com/springload/react-accessible-accordion/releases/tag/v5.0.0)

React Accessible Accordion now supports React 18 with its out-of-order streaming
Expand Down
6 changes: 3 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ needs, particularly if you're using your own `className`s.

### Accordion

#### allowMultipleExpanded : `boolean` [*optional*, default: `false`]
#### allowMultipleExpanded : `boolean` [*optional*, default: `true`]

Don't autocollapse items when expanding other items.
Don't auto-collapse items when expanding other items.

#### allowZeroExpanded : `boolean` [*optional*, default: `false`]
#### allowZeroExpanded : `boolean` [*optional*, default: `true`]

Allow the only remaining expanded item to be collapsed.

Expand Down
17 changes: 1 addition & 16 deletions demo/src/code-examples.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,6 @@ export const ExampleDefault = `import {
))}
</Accordion>`;

export const ExampleAllowMultipleExpanded = `<Accordion allowMultipleExpanded>
{items.map((item) => (
<AccordionItem key={item.uuid}>
<AccordionItemHeading>
<AccordionItemButton>
{item.heading}
</AccordionItemButton>
</AccordionItemHeading>
<AccordionItemPanel>
{item.content}
</AccordionItemPanel>
</AccordionItem>
))}
</Accordion>`;

export const ExampleAllowMultipleExpandedFalse = `<Accordion allowMultipleExpanded={false}>
{items.map((item) => (
<AccordionItem key={item.uuid}>
Expand All @@ -51,7 +36,7 @@ export const ExampleAllowMultipleExpandedFalse = `<Accordion allowMultipleExpand
))}
</Accordion>`;

export const ExampleAllowZeroExpanded = `<Accordion allowZeroExpanded>
export const ExampleAllowZeroExpandedFalse = `<Accordion allowZeroExpanded={false} preExpanded={['a']}>
{items.map((item) => (
<AccordionItem key={item.uuid}>
<AccordionItemHeading>
Expand Down
82 changes: 38 additions & 44 deletions demo/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@ import Code from './components/Code';
// tslint:disable-next-line no-import-side-effect ordered-imports
import {
ExampleDefault,
ExampleAllowMultipleExpanded,
ExampleAllowMultipleExpandedFalse,
ExampleAllowZeroExpanded,
ExampleAllowZeroExpandedFalse,
ExamplePreExpanded,
ExampleOnChange,
ExampleAccordionItemState,
Expand Down Expand Up @@ -54,8 +53,7 @@ const App = (): JSX.Element => (
<h2 className="u-margin-top">Default behaviour</h2>

<p>
By default, only one item may be expanded and it can only be
collapsed again by expanding another.
By default, any number of items may be expanded at any given time.
</p>

<Accordion>
Expand All @@ -73,32 +71,15 @@ const App = (): JSX.Element => (

<Code code={ExampleDefault} />

<h2 className="u-margin-top">Expanding multiple items at once</h2>

<p>
If you set <strong>allowMultipleExpanded</strong> to{' '}
<strong>true</strong> then the accordion will permit multiple items
to be expanded at once.
</p>

<Accordion allowMultipleExpanded={true}>
{placeholders.map((placeholder: Placeholder) => (
<AccordionItem key={placeholder.heading}>
<AccordionItemHeading>
<AccordionItemButton>
{placeholder.heading}
</AccordionItemButton>
</AccordionItemHeading>
<AccordionItemPanel>{placeholder.panel}</AccordionItemPanel>
</AccordionItem>
))}
</Accordion>

<Code code={ExampleAllowMultipleExpanded} />

<h2 className="u-margin-top">
Same as above except with allowMultipleExpanded=false
Prevent multiple items being expanded at a time
</h2>
<p>
<strong>Note:</strong> we do not recommend this behavior. Users may
wish to view the content of more than one panel at once. Also,
collapsing a panel when opening another can cause unexpected scroll
position changes.
</p>

<Accordion allowMultipleExpanded={false}>
{placeholders.map((placeholder: Placeholder) => (
Expand All @@ -115,17 +96,25 @@ const App = (): JSX.Element => (

<Code code={ExampleAllowMultipleExpandedFalse} />

<h2 className="u-margin-top">Collapsing the last expanded item</h2>
<h2 className="u-margin-top">Pre-expanded items</h2>

<p>
If you set <strong>allowZeroExpanded</strong> to{' '}
<strong>true</strong> then a solitary expanded item may be collapsed
again.
If you set <strong>preExpanded</strong>, then you can choose which
items are expanded on mount.
</p>

<p>
The strings passed to <strong>preExpanded</strong> are directly
related to the <strong>uuid</strong> props of{' '}
<strong>AccordionItem</strong>.
</p>

<Accordion allowZeroExpanded={true}>
<Accordion preExpanded={[placeholders[0].uuid]}>
{placeholders.map((placeholder: Placeholder) => (
<AccordionItem key={placeholder.heading}>
<AccordionItem
key={placeholder.heading}
uuid={placeholder.uuid}
>
<AccordionItemHeading>
<AccordionItemButton>
{placeholder.heading}
Expand All @@ -136,22 +125,27 @@ const App = (): JSX.Element => (
))}
</Accordion>

<Code code={ExampleAllowZeroExpanded} />
<Code code={ExamplePreExpanded} />

<h2 className="u-margin-top">Pre-expanded items</h2>
<h2 className="u-margin-top">Preventing the collapsing of all items</h2>

<p>
If you set <strong>preExpanded</strong>, then you can choose which
items are expanded on mount.
If you set <strong>allowZeroExpanded</strong> to{' '}
<strong>false</strong> then the user must have at least one panel
open at a time.
</p>

<p>
The strings passed to <strong>preExpanded</strong> are directly
related to the <strong>uuid</strong> props of{' '}
<strong>AccordionItem</strong>.
<strong>Note:</strong> we do not recommend this behavior. Users
would be able to expand items but not necessarily collapse them,
which might not match their expectations. If you do choose to use
this setting, we recommend you pair it with having{' '}
<strong>preExpanded</strong> item(s).
</p>

<Accordion preExpanded={[placeholders[0].uuid]}>
<Accordion
allowZeroExpanded={false}
preExpanded={[placeholders[0].uuid]}
>
{placeholders.map((placeholder: Placeholder) => (
<AccordionItem
key={placeholder.heading}
Expand All @@ -167,7 +161,7 @@ const App = (): JSX.Element => (
))}
</Accordion>

<Code code={ExamplePreExpanded} />
<Code code={ExampleAllowZeroExpandedFalse} />

<h2 className="u-margin-top">Informative onChange</h2>

Expand Down
2 changes: 1 addition & 1 deletion integration/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {

ReactDOM.render(
<div id="classic-accordion">
<Accordion>
<Accordion allowMultipleExpanded={false} allowZeroExpanded={false}>
Copy link

Choose a reason for hiding this comment

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

Shouldn't the integration tests stick to testing the default Accordian behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was either update the tests or update the accordion, and I chose the lazy option 😬

Fair call, I will take a look.

Copy link
Contributor Author

@liamjohnston liamjohnston Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, if I change it to run against the default settings, the integration test fails (as I expected).

However, the failing integration test in question is written assuming the old default behaviour. Whereas the other tests don't – they just test based on the DOM they see (e.g. if an accordion item is aria-expanded, the panel should be visible). The integration tests run in a browser (with puppeteer?).

The failing test opens the first panel, then expects that it can't be collapsed (because it's the only one open, and allowZeroExpanded:false was the default behaviour).

RAA doesn't add any DOM cues regarding allowMultipleExpanded or allowMultipleExpanded, so we can't really test against them.

I think the best course of action here is to remove that particular test. We already have an integration test that opening a panel works.

The alternative is adding more DOM guff like data-disallow-multiple-expanded/data-disallow-zero-expanded, but that feels like a too-big change to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(done)

<AccordionItem>
<AccordionItemHeading>
<AccordionItemButton>Heading One</AccordionItemButton>
Expand Down
2 changes: 1 addition & 1 deletion integration/wai-aria.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ describe('WAI ARIA Spec', () => {
const { browser, page, buttonsHandles } = await setup();
expect(buttonsHandles.length).toEqual(3);

const [firstButtonHandle] = buttonsHandles;
const firstButtonHandle = buttonsHandles[0];
await firstButtonHandle.click();

const buttonAriaDisabled = await page.evaluate(
Expand Down
6 changes: 5 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-accessible-accordion",
"version": "5.0.0",
"version": "6.0.0",
"description": "Accessible Accordion component for React",
"main": "dist/umd/index.js",
"module": "dist/es/index.js",
Expand Down Expand Up @@ -73,6 +73,10 @@
{
"name": "Emilia Zapata",
"url": "https://github.com/synecdokey"
},
{
"name": "Liam Johnston",
"url": "https://github.com/liamjohnston"
}
],
"license": "MIT",
Expand Down
22 changes: 12 additions & 10 deletions src/components/Accordion.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ describe('Accordion', () => {

describe('expanding and collapsing: ', () => {
describe('allowMultipleExpanded prop', () => {
it('permits multiple items to be expanded when explicitly true', () => {
it('prevents multiple items to be expanded when set to false', () => {
const { getByTestId } = render(
<Accordion allowMultipleExpanded={true}>
<Accordion allowMultipleExpanded={false}>
<AccordionItem>
<AccordionItemHeading>
<AccordionItemButton data-testid={UUIDS.FOO} />
Expand All @@ -60,13 +60,13 @@ describe('Accordion', () => {

expect(
getByTestId(UUIDS.FOO).getAttribute('aria-expanded'),
).toEqual('true');
).toEqual('false');
expect(
getByTestId(UUIDS.BAR).getAttribute('aria-expanded'),
).toEqual('true');
});

it('prevents multiple items being expanded by default', () => {
it('allows multiple items being expanded by default', () => {
const { getByTestId } = render(
<Accordion>
<AccordionItem>
Expand All @@ -87,17 +87,17 @@ describe('Accordion', () => {

expect(
getByTestId(UUIDS.FOO).getAttribute('aria-expanded'),
).toEqual('false');
).toEqual('true');
expect(
getByTestId(UUIDS.BAR).getAttribute('aria-expanded'),
).toEqual('true');
});
});

describe('allowZeroExpanded prop', () => {
it('permits the last-expanded item to be collapsed when explicitly true', () => {
it('prevents the last-expanded item to be collapsed when explicitly false', () => {
const { getByTestId } = render(
<Accordion allowZeroExpanded={true}>
<Accordion allowZeroExpanded={false}>
<AccordionItem>
<AccordionItemHeading>
<AccordionItemButton data-testid={UUIDS.FOO} />
Expand All @@ -106,15 +106,17 @@ describe('Accordion', () => {
</Accordion>,
);

// open it
fireEvent.click(getByTestId(UUIDS.FOO));
// attempt to close it
fireEvent.click(getByTestId(UUIDS.FOO));

expect(
getByTestId(UUIDS.FOO).getAttribute('aria-expanded'),
).toEqual('false');
).toEqual('true');
});

it('prevents the last-expanded item being collapsed by default', () => {
it('allows the last-expanded to collapsed by default', () => {
liamjohnston marked this conversation as resolved.
Show resolved Hide resolved
const { getByTestId } = render(
<Accordion>
<AccordionItem>
Expand All @@ -130,7 +132,7 @@ describe('Accordion', () => {

expect(
getByTestId(UUIDS.FOO).getAttribute('aria-expanded'),
).toEqual('true');
).toEqual('false');
});
});

Expand Down
4 changes: 2 additions & 2 deletions src/components/AccordionContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ export class Provider extends React.PureComponent<
ProviderState
> {
static defaultProps: ProviderProps = {
allowMultipleExpanded: false,
allowZeroExpanded: false,
allowMultipleExpanded: true,
allowZeroExpanded: true,
};

state: ProviderState = new AccordionStore({
Expand Down
Loading