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.

Sign up for GitHub

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

Jump to bottom

[UnderlineNav2]: Add visually hidden heading where aria-label is present #2470

Merged
merged 3 commits into from
Oct 25, 2022
Merged

[UnderlineNav2]: Add visually hidden heading where aria-label is present #2470

merged 3 commits into from
Oct 25, 2022

Conversation

broccolinisoup
Copy link
Member

@broccolinisoup broccolinisoup commented Oct 23, 2022 •
edited
Loading

Following up the accessibility sign-off review feedback (ref: comment), this PR adds a visually hidden heading where aria-label is present

Story book link: https://primer-a937bd5bd7-13348165.drafts.github.io/storybook/?path=/story/components-underlinenav--default-nav

Screenshots

A screenshot of the DOM that shows a visually hidden h2 heading as a sibling to the nav landmark

Merge checklist

  • Added/updated tests
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 5288681

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2022 •
edited
Loading

size-limit report 📦

Path Size
dist/browser.esm.js 78.12 KB (0%)
dist/browser.umd.js 78.77 KB (0%)

@broccolinisoup broccolinisoup temporarily deployed to github-pages October 23, 2022 22:32 Inactive
@broccolinisoup broccolinisoup force-pushed the introduce-disclosure-pattern branch from b115613 to 8427af9 Compare October 23, 2022 22:58
@broccolinisoup broccolinisoup force-pushed the visually-hidden-headings branch from 8a1143d to 23a3e5b Compare October 23, 2022 23:03
@broccolinisoup broccolinisoup temporarily deployed to github-pages October 23, 2022 23:10 Inactive
@broccolinisoup broccolinisoup force-pushed the introduce-disclosure-pattern branch from 8427af9 to ef2cb54 Compare October 24, 2022 01:50
@broccolinisoup broccolinisoup force-pushed the visually-hidden-headings branch from 23a3e5b to e5ff68e Compare October 24, 2022 02:06
@broccolinisoup broccolinisoup temporarily deployed to github-pages October 24, 2022 02:12 Inactive
@broccolinisoup broccolinisoup marked this pull request as ready for review October 24, 2022 02:42
ericwbailey
ericwbailey requested changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@ericwbailey ericwbailey left a comment

Choose a reason for hiding this comment

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

One small thing. Could we append navigation to the heading's string?

The reason is a nav element will be announced as a navigation landmark, but the heading does not indicate it is for navigation without the appended string suppling the context.

joshblack
joshblack approved these changes Oct 24, 2022
View reviewed changes
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Changes look great on the implementation side ✨

Is there a good way to learn more about the technique itself? Was surprised that this would work for visually hidden content but I guess it makes sense if the user can find it and then is expected to hit tab to go to following content

@ericwbailey
Copy link
Contributor

@joshblack This came up in conversation with James (not tagging him here to keep his notifications down). His point was you want to have a heading present to provide quick context to people navigating via heading, which is a very common navigation technique. It's more a quality of life improvement than it is an accessibility issue, but I think a good one!

@broccolinisoup
Copy link
Member Author

One small thing. Could we append navigation to the heading's string?

The reason is a nav element will be announced as a navigation landmark, but the heading does not indicate it is for navigation without the appended string suppling the context.

Ahh makes sense! Just pushed a commit.
A screenshot of the DOM that shows a visually hidden h2 heading as a sibling to the nav landmark

@broccolinisoup broccolinisoup temporarily deployed to github-pages October 24, 2022 21:01 Inactive
ericwbailey
ericwbailey approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@ericwbailey ericwbailey left a comment

Choose a reason for hiding this comment

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

Woohoo! 🚀

@broccolinisoup broccolinisoup force-pushed the introduce-disclosure-pattern branch from ef2cb54 to 2b1a8d2 Compare October 25, 2022 00:15
@broccolinisoup broccolinisoup force-pushed the visually-hidden-headings branch from c0fd5e0 to 5288681 Compare October 25, 2022 01:58
@broccolinisoup broccolinisoup temporarily deployed to github-pages October 25, 2022 02:06 Inactive
@broccolinisoup broccolinisoup merged commit 039b206 into introduce-disclosure-pattern Oct 25, 2022
@broccolinisoup broccolinisoup deleted the visually-hidden-headings branch October 25, 2022 02:43
broccolinisoup added a commit that referenced this pull request Oct 26, 2022
@broccolinisoup
[UnderlineNav2]: Add visually hidden heading where aria-label is pr…
858b449 
…esent (#2470)

* Add visually hidden heading

* add changeset and a test

* append 'navigation' to the aria-label string
broccolinisoup added a commit that referenced this pull request Oct 26, 2022
@broccolinisoup @siddharthkp
[UnderlineNav2]: Introduce disclosure widget pattern (#2466)
9ff856d 
* Disclosure pattern implementation

* use token name for colours instead of accessing them from theme

Co-authored-by: Siddharth Kshetrapal <[email protected]>

* code review feedback <3

* [UnderlineNav2]: Always show at least 2 items in the overflow menu (A11y remediations) (#2471)

* Display at least two items in the menu

* add changeset

* [UnderlineNav2]: Add visually hidden heading where `aria-label` is present (#2470)

* Add visually hidden heading

* add changeset and a test

* append 'navigation' to the aria-label string

* use prop for 'as'

* add changeset

Co-authored-by: Siddharth Kshetrapal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericwbailey ericwbailey ericwbailey approved these changes

@joshblack joshblack joshblack approved these changes

@josepmartins josepmartins Awaiting requested review from josepmartins

@colebemis colebemis Awaiting requested review from colebemis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@broccolinisoup @ericwbailey @joshblack