Skip to content

Conversation

@mathildebuenerd
Copy link
Contributor

@mathildebuenerd mathildebuenerd commented Jul 28, 2021

WHY are these changes introduced?

Fixes #4289
Fixes Shopify/handshake-discovery-team#247

Currently it's not possible to pass a label to the Navigation component that can be used to differentiate this nav from other nav when navigating with landmarks.

This PR is adding a way to generate the following output (described here: https://www.w3.org/WAI/tutorials/menus/structure/#label-menus):

<nav aria-labelledby="mainmenulabel">
    <h2 id="mainmenulabel" class="visuallyhidden">Main Menu</h2>
</nav>

From this implementation:

<Navigation ariaLabelledBy="mainmenulabel">
    <VisuallyHidden>
        <h2 id="mainmenulabel">Main</h2>
    </VisuallyHidden>
</Navigation>

WHAT is this pull request doing?

Adds ariaLabelledBy props to Navigation component. It is then passed as aria-labelledby property to the <nav> element.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';

import {Navigation, Page, VisuallyHidden} from '../src';

export function Playground() {
  const ariaLabelledById = 'label-1';

  const children = (
    <ul>
      <li>Item</li>
    </ul>
  );
  return (
    <Page title="Playground">
      <Navigation location="/" ariaLabelledBy={ariaLabelledById}>
        <VisuallyHidden>
          <h2 id={ariaLabelledById}>Hidden label</h2>
        </VisuallyHidden>
        {children}
      </Navigation>
    </Page>
  );
}

Tophat instructions:

  • There is not visible change in the UI
  • Use the above Playground code

1. Inspect the the HTML output:

  • The label should be not be visible and the id of the label should equal the aria-labelledby value of the <nav>

2. Use landmark navigation

  • Use Voice over and open landmarks navigation:
    • Activate Voice over (hold down the Command key and triple-tap the Touch ID button)
    • Open the rotor by pressing control + option + U
    • Use left/right arrows to find "Landmark navigation" panel
    • In this panel there should be an entry "Hidden label navigation"

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2021

size-limit report

Path Size
cjs 142.13 KB (+0.02% 🔺)
esm 95.86 KB (+0.02% 🔺)
esnext 138.87 KB (+0.01% 🔺)
css 33.56 KB (0%)

@mathildebuenerd mathildebuenerd force-pushed the 4289/navigation-aria-label branch 2 times, most recently from 28f4a7a to 94a2065 Compare July 29, 2021 07:09
@mathildebuenerd mathildebuenerd self-assigned this Jul 29, 2021
@mathildebuenerd mathildebuenerd marked this pull request as ready for review July 29, 2021 07:18
@mathildebuenerd mathildebuenerd force-pushed the 4289/navigation-aria-label branch from 94a2065 to dcfe6b3 Compare July 29, 2021 07:34
@mathildebuenerd mathildebuenerd changed the title [Navigation] Add ariaLabel and ariaLabelledBy props in order to add accessibility labels [Navigation] Add ariaLabelledBy prop in order to add menu label for screen readers Jul 29, 2021
UNRELEASED.md Outdated

- Added `id` prop to `Layout` and `Heading` for hash linking ([#4307](https://github.com/Shopify/polaris-react/pull/4307))
- Added `external` prop to `Navigation.Item` component ([#4310](https://github.com/Shopify/polaris-react/pull/4310))
- Added `ariaLabel` and `ariaLabelledBy` props to `Navigation` component in order to add labels for accessibility ([#4289](https://github.com/Shopify/polaris-react/pull/4289))
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you're only adding ariaLabelledBy. Can you remove the ariaLabel part from this PR?

Also the PR number is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I fixed both 👍

@mathildebuenerd mathildebuenerd force-pushed the 4289/navigation-aria-label branch from dcfe6b3 to 34a347f Compare July 29, 2021 14:21
Copy link
Member

@kyledurand kyledurand 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 👏 Thanks @mathildebuenerd

@mathildebuenerd mathildebuenerd merged commit a3f4700 into main Aug 2, 2021
@mathildebuenerd mathildebuenerd deleted the 4289/navigation-aria-label branch August 2, 2021 07:36
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.

[Navigation] Add ariaLabelledBy prop

2 participants