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

Accordion Adjustments #7

Merged
merged 7 commits into from
Apr 4, 2019
Merged

Accordion Adjustments #7

merged 7 commits into from
Apr 4, 2019

Conversation

dgrdl
Copy link
Contributor

@dgrdl dgrdl commented Mar 29, 2019

  • added field for hardcoded aria label removed aria label
  • refactored some code
  • added missing escaping
  • removed redundant elements
  • added inner div for content with comments in stylus to prevent coding mistakes I've seen multiple times before

@@ -1,15 +1,16 @@
<div class="flyntComponent" is="flynt-accordion-default">
<div class="container">
<div class="content">
<ul class="accordion" aria-label="Accordion Panel">
<ul class="accordion" aria-label="{{ ariaLabel|e }}">
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need this aria label at all? I don't get the accessibility value on this one and found w3 doesn't even have it in their example: https://www.w3.org/TR/wai-aria-practices/examples/accordion/accordion.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I don't really see the immediate value of this label, though I didn't test on a screen reader yet. If we're talking accessibility, this seems to also be a good summary resource for accordions: https://davatron5000.github.io/a11y-nutrition-cards/components/accordion

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it then. Makes the component and content maintainability simpler. If we find out we need it (or another aria-label), I would be happy to add it in again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is where the aria label might have originated: http://web-accessibility.carnegiemuseums.org/code/accordions/. They don't provide a reason other than "to let the user know explicitly that they are dealing with accordions" and their source U.S. Web Design System listed below the article also doesn't add an aria-label.

I removed the aria-label in the latest commit.

@dgrdl dgrdl requested a review from steffenbew April 3, 2019 15:29
@dgrdl dgrdl merged commit 2b2e6ae into master Apr 4, 2019
@dgrdl dgrdl deleted the accordion-adjustments branch April 4, 2019 14:48
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.

None yet

3 participants