Skip to content

Commit e296b63

Browse files
colebemiscolebemis
authored
Merge branch 'main' into treeview-sign-off
2 parents a8b1eab + 9edd911 commit e296b63

File tree

9 files changed

+221
-63
lines changed

9 files changed

+221
-63
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

‎.github/workflows/visual_testing.yml‎

Lines changed: 0 additions & 32 deletions
This file was deleted.

‎contributor-docs/adrs/adr-010-drafts-are-experimental.md‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
# ADR 010: Merge drafts status into experimental
2+
3+
## Status
4+
5+
Draft
6+
7+
## Context
8+
9+
`Drafts` were introduced in [ADR-006](./adrs/adr-006-drafts.md) as a way of creating components outside of the root bundle of `@primer/react`. This helps us create, test and maintain multiple versions of the same component in parallel.
10+
11+
Example:
12+
13+
```js
14+
// default version of component exported from root of package
15+
import {UnderlineNav} from '@primer/react'
16+
17+
// new version of component exported from /drafts
18+
import {UnderlineNav} from '@primer/react/drafts'
19+
```
20+
21+
We have also used this method for components that are still a work in progress and we don't want developers to start using them in production without collaborating with us first.
22+
23+
Example:
24+
25+
```js
26+
// this component only exists in drafts and not in the roof of the package
27+
import {TreeView} from '@primer/react/drafts'
28+
```
29+
30+
### Why do we not reuse `experimental` status?
31+
32+
The reason we do not use `experimental` label for these components is because `drafts` conveys a different message than maturity on the [component lifecycle](https://primer.style/contribute/component-lifecycle).
33+
34+
A `draft` component can be further along on the component lifecycle than `experimental`, but still not be the recommended version of the component until the next major release (to plan breaking changes better). For example, at the time of writing, the [new UnderlineNav2 in drafts](https://primer.style/react/drafts/UnderlineNav2) has been reviewed for accessibility and is almost at `Beta` maturity. We can call it a `draft` component with `beta` maturity.
35+
36+
### Problem
37+
38+
This is very confusing. While the above separation is useful for maintainers of primer, it is less useful to consumers.
39+
40+
1. Consumers of primer are often confused to not find `draft` maturity on the component lifecycle.
41+
2. It is similar to `experimental` label in the most important characteristic to consumers - The component is not ready for production usage.
42+
43+
### Decision
44+
45+
We should drop the `draft` label and use `experimental` instead.
46+
47+
While not completely accurate (UnderlineNav2 is much more polished than `experimental` maturity), the lack of confusion makes it a good trade-off.
48+
49+
Fine-grained information about component maturity, useful for maintainers, is also communicated by the [status checklist in the component documentation](https://primer.style/react/drafts/UnderlineNav2#status).
50+
51+
### Side effects
52+
53+
We should create a new parallel import path for `@primer/react/experimental`.
54+
55+
```diff
56+
// new version of component exported from /experimental
57+
- import { UnderlineNav } from '@primer/react/drafts'
58+
+ import { UnderlineNav } from '@primer/react/experimental'
59+
```
60+
61+
We would need to keep `@primer/react/drafts` in the short term as well for backwards compatibility.

‎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/')

0 commit comments

Comments
 (0)