Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Remove aria-hidden from scrollable navigation #2875

Merged
merged 10 commits into from
Jan 8, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/psammead-navigation/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<!-- prettier-ignore -->
| Version | Description |
|---------|-------------|
| 6.0.0-alpha.18 | [PR#2875](https://github.com/bbc/psammead/pull/2875) Remove aria-hidden from the scrollable navigation |
| 6.0.0-alpha.17 | [PR#2829](https://github.com/bbc/psammead/pull/2829) Remove references to SkipLink |
| 6.0.0-alpha.16 | [PR#2827](https://github.com/bbc/psammead/pull/2827) Talos - Bump Dependencies - @bbc/psammead-assets |
| 6.0.0-alpha.15 | [PR#2802](https://github.com/bbc/psammead/pull/2802) Spread extra Navigation props |
Expand Down
8 changes: 1 addition & 7 deletions packages/components/psammead-navigation/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ The `@bbc/psammead-navigation` package is a set of two components, `NavigationUl
| Argument | Type | Required | Default | Example |
| -------- | ---- | -------- | ------- | ------- |
| children | node | Yes | N/A | `<NavigationLi url="/" script={latin} active="true">Home</NavigationLi><NavigationLi url="/sport" script={latin}>{Sport}</NavigationLi>` |
| isScrollable | boolean | No | `false` | `true` |

### NavigationLi

Expand All @@ -48,7 +47,6 @@ The `@bbc/psammead-navigation` package is a set of two components, `NavigationUl
| currentPageText | string | No | `null` | `Current page` |
| service | string | Yes | N/A | `'news'` |
| dir | string | No | `'ltr'` | `'rtl'` |
| isScrollable | boolean | No | `false` | `true` |

### CanonicalScrollableNavigation

Expand All @@ -57,7 +55,6 @@ The `@bbc/psammead-navigation` package is a set of two components, `NavigationUl
| -------- | ---- | -------- | ------- | ------- |
| children | node | Yes | N/A | `<NavigationUl><NavigationLi url="/" script={latin} active="true">Home</NavigationLi><NavigationLi url="/sport" script={latin}>{Sport}</NavigationLi></NavigationUl>` |
| dir | string | No | `'ltr'` | `'rtl'` |
| isScrollable | boolean | No | `false` | `true` |

### AmpScrollableNavigation

Expand Down Expand Up @@ -204,6 +201,7 @@ import { latin } from '@bbc/gel-foundations/scripts';
dir={dir}
/>
```

Note that in order for the `AmpMenuButton` toggling to work correctly, an `id` should be added to the `Navigation` component. This `id` can be passed in as a prop to the component. Similarly, `AmpScrollableNavigation` also requires an `id` to be added to it.

### When to use this component
Expand All @@ -222,10 +220,6 @@ We have also added visually hidden text to let the user know which item in both

We have added an `aria-expanded` attribute to the menu button to indicate whether the menu is collapsed or expanded.

In the screen reader UX only the menu button and its content should be available to assistive technology, meaning the scrollable navigation will be hidden. To achieve this we add `aria-hidden:true` to the exposed scrollable navigation so that this is not visible to these users and also add `tabindex=-1` to the links contained within this to remove them from the tab order.

On the other hand, when Javascript is disabled, the window object will not be defined and the `useMediaQuery` will return null so `isScrollable` will be null too, therefore the scrollable navigation will be fully available to keyboard users via the tab key and to screen reader users.

## Contributing

Psammead is completely open source. We are grateful for any contributions, whether they be new components, bug fixes or general improvements. Please see our primary contributing guide which can be found at [the root of the Psammead respository](https://github.com/bbc/psammead/blob/latest/CONTRIBUTING.md).
Expand Down
2 changes: 1 addition & 1 deletion packages/components/psammead-navigation/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/components/psammead-navigation/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@bbc/psammead-navigation",
"version": "6.0.0-alpha.17",
"version": "6.0.0-alpha.18",
"description": "A navigation bar to use on index pages",
"main": "dist/index.js",
"module": "esm/index.js",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import styled, { css } from 'styled-components';
import { node, oneOf, bool } from 'prop-types';
import { node, oneOf } from 'prop-types';
import { GEL_GROUP_2_SCREEN_WIDTH_MAX } from '@bbc/gel-foundations/breakpoints';

/* Convert C_POSTBOX to rgba as IE doesn't like 8 digit hex */
Expand Down Expand Up @@ -42,30 +42,13 @@ const StyledScrollableNav = styled.div`
}
`;

export const CanonicalScrollableNavigation = ({
children,
dir,
isScrollable,
}) => {
const ariaHidden = isScrollable && { 'aria-hidden': true };

return (
<StyledScrollableNav dir={dir} {...ariaHidden}>
{React.Children.map(children, child =>
React.cloneElement(child, { isScrollable }),
)}
</StyledScrollableNav>
);
};
export const CanonicalScrollableNavigation = ({ children, dir }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now exactly the same as the Amp scrollable nav right? And we probably need the spread props for canonical too for event tracking? Should we just merge them?

<StyledScrollableNav dir={dir}>{children}</StyledScrollableNav>
);

CanonicalScrollableNavigation.propTypes = {
children: node.isRequired,
dir: oneOf(['ltr', 'rtl']),
isScrollable: bool,
};

CanonicalScrollableNavigation.defaultProps = {
isScrollable: false,
};

CanonicalScrollableNavigation.defaultProps = { dir: 'ltr' };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1653,7 +1653,6 @@ exports[`Scrollable Navigation should render scrollable Canonical version correc
}

<div
aria-hidden="true"
class="c0"
dir="ltr"
>
Expand Down
25 changes: 4 additions & 21 deletions packages/components/psammead-navigation/src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -141,21 +141,16 @@ CurrentLink.defaultProps = {
currentPageText: null,
};

export const NavigationUl = ({ children, isScrollable, ...props }) => (
<StyledUnorderedList role="list" isScrollable={isScrollable} {...props}>
{React.Children.map(children, child =>
React.cloneElement(child, { isScrollable }),
)}
export const NavigationUl = ({ children, ...props }) => (
<StyledUnorderedList role="list" {...props}>
{children}
</StyledUnorderedList>
);

NavigationUl.propTypes = {
children: node.isRequired,
isScrollable: bool,
};

NavigationUl.defaultProps = { isScrollable: false };

export const NavigationLi = ({
children: link,
url,
Expand All @@ -164,11 +159,8 @@ export const NavigationLi = ({
active,
service,
dir,
isScrollable,
...props
}) => {
const tabIndex = isScrollable && { tabIndex: -1 };

return (
<StyledListItem dir={dir} role="listitem">
{active && currentPageText ? (
Expand All @@ -177,21 +169,14 @@ export const NavigationLi = ({
script={script}
service={service}
currentLink
{...tabIndex}
{...props}
>
<CurrentLink script={script} currentPageText={currentPageText}>
{link}
</CurrentLink>
</StyledLink>
) : (
<StyledLink
href={url}
script={script}
service={service}
{...tabIndex}
{...props}
>
<StyledLink href={url} script={script} service={service} {...props}>
{link}
</StyledLink>
)}
Expand All @@ -206,14 +191,12 @@ NavigationLi.propTypes = {
active: bool,
currentPageText: string,
service: string.isRequired,
isScrollable: bool,
dir: oneOf(['ltr', 'rtl']),
};

NavigationLi.defaultProps = {
active: false,
currentPageText: null,
isScrollable: false,
dir: 'ltr',
};

Expand Down
15 changes: 1 addition & 14 deletions packages/components/psammead-navigation/src/index.stories.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import React from 'react';
import { storiesOf } from '@storybook/react';
import styled from 'styled-components';
import { useEffect, useState } from '@storybook/addons';
import {
color,
select,
Expand Down Expand Up @@ -143,24 +142,12 @@ const navigationStory = (currentPageText, navData, dir, brand, isAmp) => ({
? AmpScrollableNavigation
: CanonicalScrollableNavigation;

const [isScrollable, setIsScrollable] = useState(false);

useEffect(() => {
const mediaQueryList = window.matchMedia('(max-width: 37.5rem)');
setIsScrollable(mediaQueryList.matches);

const handler = event => setIsScrollable(event.matches);
mediaQueryList.addListener(handler);

return () => mediaQueryList.removeListener(handler);
}, []);

return (
<>
{brand && getBrand()}

<Navigation script={script} service={service} dir={dir}>
<ScrollableNavigation dir={dir} isScrollable={isScrollable}>
<ScrollableNavigation dir={dir}>
<NavigationUl>
{navData.map((item, index) => {
const { title, url } = item;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/psammead-navigation/src/index.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('Navigation', () => {
describe('Scrollable Navigation', () => {
shouldMatchSnapshot(
'should render scrollable Canonical version correctly',
<CanonicalScrollableNavigation isScrollable>
<CanonicalScrollableNavigation>
{NavigationExample}
</CanonicalScrollableNavigation>,
);
Expand Down