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: Updating NavLinkButton to announce current page #4677

Merged
merged 24 commits into from
Sep 13, 2021

Conversation

JGibson2019
Copy link
Contributor

@JGibson2019 JGibson2019 commented Sep 10, 2021

Details

When using the Left Nav from results view in Unified, JAWS/NVDA users were not able to know if they had navigated to a list item that was selected or unselected. By adding the "aria-current" attribute to each item, the selected list item is announced as "current page", so they have the necessary context.

Motivation

Addresses the following Unified bug: https://dev.azure.com/mseng/1ES/_workitems/edit/1859838

Context
  • Rather than have to add the full code ourselves, we updated Office UI Fabric React from 7.98.0 to version 7.98.1 so that we can utilize the following PR (Use aria-current for active nav links fluentui#11789)
  • This affects the LeftNav experience in the web extension, but testing verified that the "current page" announcement occurs there.

Pull request checklist

  • Addresses an existing issue: #0000 - https://dev.azure.com/mseng/1ES/_workitems/edit/1859838
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • [N/A ] Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • [N/A] (UI changes only) Added screenshots/GIFs to description above
  • (UI changes only) Verified usability with NVDA/JAWS

JGibson2019 and others added 21 commits July 24, 2020 09:42
…dated the snapshots for their unit tests to account for the new text
@JGibson2019 JGibson2019 changed the title fix: Updating NavLinkButton to utilize the "aria-current" property to announce current page fix: Updating NavLinkButton to announce current page Sep 10, 2021
@JGibson2019 JGibson2019 marked this pull request as ready for review September 13, 2021 17:35
@JGibson2019 JGibson2019 requested a review from a team as a code owner September 13, 2021 17:35
@@ -2187,11 +2187,16 @@
resolved "https://registry.yarnpkg.com/@types/ms/-/ms-0.7.31.tgz#31b7ca6407128a3d2bbc27fe2d21b345397f6197"
integrity sha512-iiUgKzV9AuaEkZqkOLDIvlQiL6ltuZd9tGcW3gwpnX8JbuiuhFlEGmmFXEXkN50Cvq7Os88IY2v0dkDqXYWVgA==

"@types/node@*", "@types/node@>= 8", "@types/node@^12.0.12", "@types/node@^14.14.6", "@types/node@^14.17.15":
"@types/node@*", "@types/node@>= 8", "@types/node@^14.17.15":
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these changes to yarn.lock expected? Many don't seem connected to the office-ui-fabric-react update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. When I bumped the office-ui-fabric-react version, it appears that it updated also updated the yarn.lock entries for packages that are dependencies. I believe this happened in the "linking dependencies" and "build fresh packages" steps; that resulted in the reformatting (e.g. moving @types/node@^12.0.12" from line 2190 and creating a separate entry below on 2195.

Copy link
Contributor

@peterdur peterdur left a comment

Choose a reason for hiding this comment

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

Read through, ran and verified behavior.
Approving with one question in yarn.lock.

@JGibson2019 JGibson2019 merged commit 978f142 into microsoft:main Sep 13, 2021
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.

2 participants