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 nav header accessibility issues #6538

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Conversation

emyl3
Copy link
Collaborator

@emyl3 emyl3 commented Sep 12, 2023

FRONTEND PULL REQUEST

Related Issue

Resolves #6410

Changes Proposed

Screenshot 2023-09-11 at 23 00 32
  • Account icon in header
  • Fix other accessibility issues found when adding jest-axe test to <Header> component

Additional Information

  • Adding the jest-axe test to the <Header> component revealed that the axe dev tool test didn't run the tests on certain parts of the nav header unless the Account icon button was clicked and expanded.
  • Some changes in this PR addresses the issues flagged by the jest-axe tests

Testing

  • deployed on dev2

Screenshots / Demos

Before

Screenshot 2023-09-11 at 22 56 28

After

Screenshot 2023-09-11 at 22 55 51

⚠️ The line styling in between the "USER NAME" and "USER ROLE" in the Account dropdown has changed slightly since we cannot use <hr> element in a <ul> element

Screenshot 2023-09-11 at 22 55 16

>
Login as {name}
</a>
<li>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ caught by axe-jest, cannot have <li> element within <ul> parent element

Copy link
Contributor

Choose a reason for hiding this comment

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

just to clarify, did you mean that we can't have a <a> element within a <ul>? wanted to make sure I understood for future reference!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ooo sorry I missed this comment. I think there were only three elements that could be direct children of the <ul> element... let me try to see if I can get that error again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -91,6 +93,7 @@ const Dropdown: React.FC<Props & SelectProps> = ({
)}
name={name}
id={id}
aria-label={ariaLabel}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ caught by axe-jest, need to have some sort of label for <select>

@emyl3 emyl3 force-pushed the elisa/6410-header-user-submenu-acc branch from 469fc91 to 14bbb32 Compare September 12, 2023 04:12
ref={staffDefailsRef}
aria-label="Primary navigation"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ caught by axe-jest, cannot repeat same aria-label

@emyl3 emyl3 force-pushed the elisa/6410-header-user-submenu-acc branch from 14bbb32 to 3c4bcdb Compare September 12, 2023 14:05
@emyl3 emyl3 temporarily deployed to dev2 September 12, 2023 14:16 — with GitHub Actions Inactive
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@emyl3 emyl3 marked this pull request as ready for review September 12, 2023 14:32
Copy link
Contributor

@fzhao99 fzhao99 left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@BobanL BobanL left a comment

Choose a reason for hiding this comment

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

looks good to me!

@emyl3 emyl3 added this pull request to the merge queue Sep 12, 2023
Merged via the queue into main with commit 5827e71 Sep 12, 2023
@emyl3 emyl3 deleted the elisa/6410-header-user-submenu-acc branch September 12, 2023 15:32
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.

[Main menu - Site Admin] account menu item role should be button and collapse attribute properly populated
3 participants