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.

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

Fix accordion state #107

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix accordion state #107

wants to merge 1 commit into from

Conversation

gunzip
Copy link
Contributor

@gunzip gunzip commented May 28, 2017

Sometimes there's the need for accordion panels to be opened by default.
ie. to preserve state between interactions. This PR aims to maintain accordion panels open at init()
when the controlling header is marked with aria-selected="true"

@adamduncan
Copy link
Contributor

Thanks @gunzip, will check this out. I think this is a great idea. Do you think it's worth getting devs to manually set the active panel as aria-hidden="false" to cover both bases, or just infer active panel solely based on headers? We'd need to make it clear in docs that this functionality is available (and not achieved using aria-expanded="true" on the headers, for example).

@thomasdigby, this might get around the need for the firstPanelsOpenByDefault option in future versions.

@gunzip
Copy link
Contributor Author

gunzip commented Jun 6, 2017

Do you think it's worth getting devs to manually set the active panel as aria-hidden="false" to cover both bases, or just infer active panel solely based on headers?

probably it's more practical to infer the active panel based solely on headers
as the aria-hidden attribute gets set anyway (but I'm not biased)

We'd need to make it clear in docs that this functionality is available (and not achieved using aria-expanded="true" on the headers, for example).

that's right. maybe it's worth to activate the 'open' state checking for aria-expanded="true" as well ? (dunno...)

@francescozaia
Copy link

Hey @adamduncan this one seems a bit stale, is there any chance of getting it merged or does it require additional work? Thanks!

@adamduncan
Copy link
Contributor

Sorry, you're right. I'll have a review and merge or feedback! Thanks

Copy link
Contributor

@adamduncan adamduncan 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 this update @gunzip. Everything looks good from a functionality point of view.

We'd want to document how the change can be utilised (with aria-selected and aria-hidden attributes on header/panel), and how it effects the firstPanelIsOpenByDefault option.

Also, worth a version bump to 1.1.0, a gulp build, and a note in the changelog for the component. 👍 Sorry this took so long for me to get around to.

@adamduncan
Copy link
Contributor

Here's some documentation to add to the end of the _components/accordion/README.md file (or if you like I can make the changes if you're allowing edits from maintainers):

### Setting an open panel

By default, the first panel of an accordion will be open on initialisation. To set this as a different panel, include an `aria-selected="true"` attribute on the header, and an `aria-hidden="false"` attribute on its respective panel. For example, to initialise the accordion with the third panel open by default:

~~~ html
<div class="fr-accordion js-fr-accordion">
	<h2 id="accordion-header-1" class="fr-accordion__header js-fr-accordion__header">...</h2>
	<div id="accordion-panel-1" class="fr-accordion__panel js-fr-accordion__panel">
		...
	</div>
	<h2 id="accordion-header-2" class="fr-accordion__header js-fr-accordion__header">...</h2>
	<div id="accordion-panel-2" class="fr-accordion__panel js-fr-accordion__panel">
		...
	</div>
	<h2 aria-selected="true" id="accordion-header-3" class="fr-accordion__header js-fr-accordion__header">...</h2>
	<div aria-hidden="false" id="accordion-panel-3" class="fr-accordion__panel js-fr-accordion__panel">
		...
	</div>
</div>
~~~

**Note:** In order for this functionality to work as expected, the accordion would also require the option parameter `firstPanelsOpenByDefault` set to `false` (see above Options).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants