Skip to content

Commit 3768cd7

Browse files
broccolinisoupbroccolinisoup
authored
PageHeader: A11y eng review remediations (#2900)
* a11y remediations * Add changeset * Add tests for a11y remediations * add docs for a11y remediations * Update generated/components.json * wrap console.log with __DEV__ and add a todo * fix merge issue * update docs --------- Co-authored-by: broccolinisoup <broccolinisoup@users.noreply.github.com>
1 parent 03ebf41 commit 3768cd7

File tree

7 files changed

+160
-31
lines changed

7 files changed

+160
-31
lines changed

‎.changeset/silent-rivers-sell.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@primer/react': patch
3+
---
4+
5+
PageHeader: A11y eng review remediations

‎docs/content/drafts/PageHeader.mdx

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,8 @@ import {PageHeader} from '@primer/react/drafts'
121121

122122
### With navigation slot
123123

124+
#### Utilizing a Navigation component
125+
124126
```jsx live drafts
125127
<PageHeader>
126128
<PageHeader.TitleArea>
@@ -145,6 +147,28 @@ import {PageHeader} from '@primer/react/drafts'
145147
</PageHeader>
146148
```
147149

150+
#### Utilizing a custom navigation
151+
152+
```jsx live drafts
153+
<PageHeader>
154+
<PageHeader.TitleArea>
155+
<PageHeader.Title>Page Title</PageHeader.Title>
156+
</PageHeader.TitleArea>
157+
<PageHeader.Navigation as="nav" aria-label="Item list">
158+
<Box as="ul" sx={{display: 'flex', gap: '8px', listStyle: 'none', paddingY: 0, paddingX: 3}} role="list">
159+
<li>
160+
<Link href="#" aria-current="page">
161+
Item 1
162+
</Link>
163+
</li>
164+
<li>
165+
<Link href="#">Item 2</Link>
166+
</li>
167+
</Box>
168+
</PageHeader.Navigation>
169+
</PageHeader>
170+
```
171+
148172
### With ContextArea
149173

150174
#### With parent link and actions (only visible on mobile)

‎generated/components.json

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3018,7 +3018,7 @@
30183018
]
30193019
},
30203020
{
3021-
"name": "PageHeader.ContextArea Children",
3021+
"name": "PageHeader.ParentLink",
30223022
"props": [
30233023
{
30243024
"name": "href",
@@ -3039,7 +3039,7 @@
30393039
]
30403040
},
30413041
{
3042-
"name": "PageHeader.ContextArea Children",
3042+
"name": "PageHeader.ContextBar",
30433043
"props": [
30443044
{
30453045
"name": "hidden",
@@ -3054,7 +3054,7 @@
30543054
]
30553055
},
30563056
{
3057-
"name": "PageHeader.ContextArea Children",
3057+
"name": "PageHeader.ContextAreaActions",
30583058
"props": [
30593059
{
30603060
"name": "hidden",
@@ -3090,7 +3090,7 @@
30903090
]
30913091
},
30923092
{
3093-
"name": "PageHeader.TitleArea Children",
3093+
"name": "PageHeader.LeadingAction",
30943094
"props": [
30953095
{
30963096
"name": "hidden",
@@ -3105,7 +3105,7 @@
31053105
]
31063106
},
31073107
{
3108-
"name": "PageHeader.TitleArea Children",
3108+
"name": "PageHeader.LeadingVisual",
31093109
"props": [
31103110
{
31113111
"name": "hidden",
@@ -3120,7 +3120,7 @@
31203120
]
31213121
},
31223122
{
3123-
"name": "PageHeader.TitleArea Children",
3123+
"name": "PageHeader.Title",
31243124
"props": [
31253125
{
31263126
"name": "hidden",
@@ -3140,7 +3140,7 @@
31403140
]
31413141
},
31423142
{
3143-
"name": "PageHeader.TitleArea Children",
3143+
"name": "PageHeader.TrailingVisual",
31443144
"props": [
31453145
{
31463146
"name": "hidden",
@@ -3155,7 +3155,7 @@
31553155
]
31563156
},
31573157
{
3158-
"name": "PageHeader.TitleArea Children",
3158+
"name": "PageHeader.TrailingAction",
31593159
"props": [
31603160
{
31613161
"name": "hidden",
@@ -3170,7 +3170,7 @@
31703170
]
31713171
},
31723172
{
3173-
"name": "PageHeader.TitleArea Children",
3173+
"name": "PageHeader.Actions",
31743174
"props": [
31753175
{
31763176
"name": "hidden",
@@ -3202,6 +3202,22 @@
32023202
{
32033203
"name": "PageHeader.Navigation",
32043204
"props": [
3205+
{
3206+
"name": "aria-label",
3207+
"type": "string",
3208+
"description": "The aria-label attribute for the navigaton component when it is rendered as a \"nav\" element."
3209+
},
3210+
{
3211+
"name": "aria-labelledby",
3212+
"type": "string",
3213+
"description": "The aria-labelledby attribute for the navigaton component when it is rendered as a \"nav\" element."
3214+
},
3215+
{
3216+
"name": "as",
3217+
"type": "div | nav",
3218+
"defaultValue": "\"div\"",
3219+
"description": "The HTML element used to render the navigation."
3220+
},
32053221
{
32063222
"name": "hidden",
32073223
"type": "| boolean | { narrow?: boolean regular?: boolean wide?: boolean }",

‎src/PageHeader/PageHeader.docs.json

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@
4444
]
4545
},
4646
{
47-
"name": "PageHeader.ContextArea Children",
47+
"name": "PageHeader.ParentLink",
4848
"props": [
4949
{
5050
"name": "href",
@@ -65,7 +65,7 @@
6565
]
6666
},
6767
{
68-
"name": "PageHeader.ContextArea Children",
68+
"name": "PageHeader.ContextBar",
6969
"props": [
7070
{
7171
"name": "hidden",
@@ -80,7 +80,7 @@
8080
]
8181
},
8282
{
83-
"name": "PageHeader.ContextArea Children",
83+
"name": "PageHeader.ContextAreaActions",
8484
"props": [
8585
{
8686
"name": "hidden",
@@ -116,7 +116,7 @@
116116
]
117117
},
118118
{
119-
"name": "PageHeader.TitleArea Children",
119+
"name": "PageHeader.LeadingAction",
120120
"props": [
121121
{
122122
"name": "hidden",
@@ -131,7 +131,7 @@
131131
]
132132
},
133133
{
134-
"name": "PageHeader.TitleArea Children",
134+
"name": "PageHeader.LeadingVisual",
135135
"props": [
136136
{
137137
"name": "hidden",
@@ -146,7 +146,7 @@
146146
]
147147
},
148148
{
149-
"name": "PageHeader.TitleArea Children",
149+
"name": "PageHeader.Title",
150150
"props": [
151151
{
152152
"name": "hidden",
@@ -166,7 +166,7 @@
166166
]
167167
},
168168
{
169-
"name": "PageHeader.TitleArea Children",
169+
"name": "PageHeader.TrailingVisual",
170170
"props": [
171171
{
172172
"name": "hidden",
@@ -181,7 +181,7 @@
181181
]
182182
},
183183
{
184-
"name": "PageHeader.TitleArea Children",
184+
"name": "PageHeader.TrailingAction",
185185
"props": [
186186
{
187187
"name": "hidden",
@@ -196,7 +196,7 @@
196196
]
197197
},
198198
{
199-
"name": "PageHeader.TitleArea Children",
199+
"name": "PageHeader.Actions",
200200
"props": [
201201
{
202202
"name": "hidden",
@@ -228,6 +228,22 @@
228228
{
229229
"name": "PageHeader.Navigation",
230230
"props": [
231+
{
232+
"name": "aria-label",
233+
"type": "string",
234+
"description": "The aria-label attribute for the navigaton component when it is rendered as a \"nav\" element."
235+
},
236+
{
237+
"name": "aria-labelledby",
238+
"type": "string",
239+
"description": "The aria-labelledby attribute for the navigaton component when it is rendered as a \"nav\" element."
240+
},
241+
{
242+
"name": "as",
243+
"type": "div | nav",
244+
"defaultValue": "\"div\"",
245+
"description": "The HTML element used to render the navigation."
246+
},
231247
{
232248
"name": "hidden",
233249
"type": "| boolean | { narrow?: boolean regular?: boolean wide?: boolean }",
@@ -241,4 +257,4 @@
241257
]
242258
}
243259
]
244-
}
260+
}

‎src/PageHeader/PageHeader.test.tsx

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,36 @@ describe('PageHeader', () => {
178178
expect(getByText('Trailing Action')).toHaveStyle('height: 3rem')
179179
// add actions here
180180
})
181+
it('renders "aria-label" prop when Navigation is rendered as "nav" landmark', () => {
182+
const {getByLabelText, getByText} = render(
183+
<PageHeader>
184+
<PageHeader.Navigation as="nav" aria-label="Custom">
185+
Navigation
186+
</PageHeader.Navigation>
187+
</PageHeader>,
188+
)
189+
expect(getByLabelText('Custom')).toBeInTheDocument()
190+
expect(getByText('Navigation')).toHaveAttribute('aria-label', 'Custom')
191+
})
192+
it('does not renders "aria-label" prop when Navigation is rendered as "div"', () => {
193+
const {getByText} = render(
194+
<PageHeader>
195+
<PageHeader.Navigation aria-label="Custom">Navigation</PageHeader.Navigation>
196+
</PageHeader>,
197+
)
198+
expect(getByText('Navigation')).not.toHaveAttribute('aria-label')
199+
})
200+
it('logs a warning when the Navigation component is rendered as "nav" but no "aria-label" or "aria-labelledby" prop is provided', () => {
201+
const consoleSpy = jest.spyOn(global.console, 'warn').mockImplementation()
202+
render(
203+
<PageHeader>
204+
<PageHeader.Navigation as="nav">Navigation</PageHeader.Navigation>
205+
</PageHeader>,
206+
)
207+
expect(consoleSpy).toHaveBeenCalled()
208+
209+
consoleSpy.mockRestore()
210+
})
181211
})
182212

183213
checkStoriesForAxeViolations('examples', '../PageHeader/')

‎src/PageHeader/PageHeader.tsx

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -101,17 +101,7 @@ type LinkProps = Pick<
101101
export type ParentLinkProps = React.PropsWithChildren<ChildrenPropTypes & LinkProps>
102102

103103
const ParentLink = React.forwardRef<HTMLAnchorElement, ParentLinkProps>(
104-
(
105-
{
106-
children,
107-
sx = {},
108-
href,
109-
'aria-label': ariaLabel = `Back to ${children}`,
110-
as = 'a',
111-
hidden = hiddenOnRegularAndWide,
112-
},
113-
ref,
114-
) => {
104+
({children, sx = {}, href, 'aria-label': ariaLabel, as = 'a', hidden = hiddenOnRegularAndWide}, ref) => {
115105
return (
116106
<>
117107
<Link
@@ -437,10 +427,36 @@ const Description: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({chil
437427
)
438428
}
439429

430+
export type NavigationProps = {
431+
as?: 'nav' | 'div'
432+
'aria-label'?: React.AriaAttributes['aria-label']
433+
'aria-labelledby'?: React.AriaAttributes['aria-labelledby']
434+
} & ChildrenPropTypes
435+
440436
// PageHeader.Navigation: The local navigation area of the header. Visible on all viewports
441-
const Navigation: React.FC<React.PropsWithChildren<ChildrenPropTypes>> = ({children, sx = {}, hidden = false}) => {
437+
const Navigation: React.FC<React.PropsWithChildren<NavigationProps>> = ({
438+
children,
439+
sx = {},
440+
hidden = false,
441+
as,
442+
'aria-label': ariaLabel,
443+
'aria-labelledby': ariaLabelledBy,
444+
}) => {
445+
// TODO: use warning utility function when it is merged https://github.com/primer/react/pull/2901/
446+
if (__DEV__) {
447+
if (as === 'nav' && !ariaLabel && !ariaLabelledBy) {
448+
// eslint-disable-next-line no-console
449+
console.warn(
450+
'Use `aria-label` or `aria-labelledby` prop to provide an accessible label to the `nav` landmark for assistive technology',
451+
)
452+
}
453+
}
442454
return (
443455
<Box
456+
as={as}
457+
// Render `aria-label` and `aria-labelledby` only on `nav` elements
458+
aria-label={as === 'nav' ? ariaLabel : undefined}
459+
aria-labelledby={as === 'nav' ? ariaLabelledBy : undefined}
444460
sx={merge<BetterSystemStyleObject>(
445461
{
446462
display: 'flex',

‎src/PageHeader/features.stories.tsx

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,28 @@ export const WithNavigationSlot = () => (
147147
</Box>
148148
)
149149

150+
export const WithCustomNavigation = () => (
151+
<Box sx={{padding: 3}}>
152+
<PageHeader>
153+
<PageHeader.TitleArea>
154+
<PageHeader.Title>Pull request title</PageHeader.Title>
155+
</PageHeader.TitleArea>
156+
<PageHeader.Navigation as="nav" aria-label="Item list">
157+
<Box as="ul" sx={{display: 'flex', gap: '8px', listStyle: 'none', paddingY: 0, paddingX: 3}} role="list">
158+
<li>
159+
<Link href="#" aria-current="page">
160+
Item 1
161+
</Link>
162+
</li>
163+
<li>
164+
<Link href="#">Item 2</Link>
165+
</li>
166+
</Box>
167+
</PageHeader.Navigation>
168+
</PageHeader>
169+
</Box>
170+
)
171+
150172
export const WithLeadingAndTrailingActions = () => (
151173
<Box sx={{padding: 3}}>
152174
<PageHeader>

0 commit comments

Comments
 (0)