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

Update Accordion example code and behavior #1830

Merged
merged 1 commit into from
Oct 4, 2021
Merged

Conversation

smhigley
Copy link
Contributor

@smhigley smhigley commented Mar 24, 2021

Resolves #1819 with the following changes:

Preview link

General cleanup:

  • Updates JS file to a class syntax
  • Update classnames to be lowercase
  • Fix some border-radius issues in focus styling
  • Updated example page wrapper id attributes to match other example pages

Behavior changes:

  • Removed arrow key and force-one-open functionality
  • Updated tests to reflect behavior change
  • Removed section that talked about focus styling for "enhanced keyboard interaction"
  • Removed arrow key/home/end rows in example page keyboard table
  • Removed aria-disabled row in example page attributes table

Review checklist

@carmacleod
Copy link
Contributor

10-second review: this is so much nicer than what was there before! Arrow keys are not needed when any section can be collapsed.
Thanks, @smhigley !

I guess we need the whole review checklist thing.

@smhigley
Copy link
Contributor Author

smhigley commented Mar 24, 2021

Thanks for the review and adding the preview link @carmacleod! Somehow I always forget that 😅

… arrow key nav and forced single open state in the basic example.
@shirsha
Copy link

shirsha commented Jun 15, 2021

Thanks @smhigley for the changes.
Following are few suggestion. These were existing from previous code

  • Move the "+" and "-" before the accordion name so that users with screen magnifiers can see state easily.
  • Place the accordions in an unorder list so that it improves the experience for screen reader users.
  • Can we add note saying having the content under each accordion inside a labelled region is a best practice? Having too many landmarks will not help screen reader users. I recommend removing the markup related to region landmark.

@jongund
Copy link
Contributor

jongund commented Jun 15, 2021

@smhigley
Same problem as the other accordion examples for VO on macOS/Safari, when navigating using VO next/prev item, the h3 is announced, but not the button element information.

@jongund
Copy link
Contributor

jongund commented Jun 15, 2021

@smhigley
Works fine for VO/safari for iOS 14.4.

@a11ydoer a11ydoer requested review from jesdaigle and shirsha and removed request for ZoeBijl July 13, 2021 18:11
@jongund jongund self-requested a review July 13, 2021 18:12
@a11ydoer
Copy link
Contributor

@mcking65 @shirsha @jesdaigle

Can we have update on review so that we can see whether it can be merged? If not, we can add this PR to next week meeting agenda since we will review another accordion PR 1834

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Editorial review done; looks good!

@mcking65 mcking65 merged commit d0299b2 into main Oct 4, 2021
@mcking65 mcking65 deleted the accordion-defaults branch October 4, 2021 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants