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 menu to be more accessible #369

Merged
merged 9 commits into from
Aug 9, 2021
Merged

Update menu to be more accessible #369

merged 9 commits into from
Aug 9, 2021

Conversation

jasonnrosa
Copy link
Member

@jasonnrosa jasonnrosa commented Aug 3, 2021

Types of change

  • Bug fix (change which fixes an issue)
  • Enhancement (change which improves upon an existing feature)
  • New feature (change which adds new functionality)

Description

Updates the menu module to be more accessible (e.g. aria attributes change based on hover/focus, the menu is keyboard accessible without having to tab through every menu item, etc.). This also cleans up the CSS classes used to follow BEM (was previously not closely following BEM) and cleans up code for the module in general to ideally make it simpler/easier to read and use. I generally tried to keep the look of the menu as close as I could to the original with some minor exceptions (e.g flyouts on drop down menus for desktop).

Relevant links

Fixes #286

Checklist

People to notify
CC: @TheWebTech

@jasonnrosa jasonnrosa marked this pull request as draft August 3, 2021 19:57
@jasonnrosa jasonnrosa marked this pull request as ready for review August 4, 2021 15:33
@jasonnrosa jasonnrosa mentioned this pull request Aug 6, 2021
6 tasks
@jevenson
Copy link

jevenson commented Aug 9, 2021

I'm seeing this "Show submenu" button visible, when I think its supposed to only be read by a screen reader. Not really sure what the cause is.

image

@jasonnrosa
Copy link
Member Author

@jevenson great catch - I'm not really positive why that wasn't popping up when testing locally but it looks like it was an issue with the folder name of utilities not matching the include file path in main.css. I was using a helper class on the show for submenu (so it is visible for screenreaders but not visually) but that helper class wasn't getting picked up because of the spelling error in the folder.

Copy link

@jevenson jevenson left a comment

Choose a reason for hiding this comment

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

Fix looks good! Thanks!

@jasonnrosa jasonnrosa merged commit d912a51 into main Aug 9, 2021
@jasonnrosa jasonnrosa deleted the update-menu branch August 9, 2021 19:07
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.

Rebuild the menu module to be more accessible
3 participants