Skip to content

Commit f0de234

Browse files
joshblackkhiga8
joshblack
and
khiga8
authored
refactor(Banner): update region to use a dedicated aria-label (#4539)
* refactor(Banner): update region to use a dedicated aria-label * chore: add changeset, update config to exclude codesandbox * feat: add aria-label to Banner * Update packages/react/src/Banner/Banner.test.tsx Co-authored-by: Kate Higa <[email protected]> * Update packages/react/src/Banner/Banner.test.tsx Co-authored-by: Kate Higa <[email protected]> * test: update test label with aria-label --------- Co-authored-by: Josh Black <[email protected]> Co-authored-by: Kate Higa <[email protected]>
1 parent 9a56727 commit f0de234

File tree

7 files changed

+146
-72
lines changed

7 files changed

+146
-72
lines changed

‎.changeset/cold-starfishes-shout.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@primer/react': minor
3+
---
4+
5+
Update Banner to use an explicit aria-label instead of being labelled by Banner title

‎.changeset/config.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,5 +6,5 @@
66
"access": "public",
77
"baseBranch": "main",
88
"updateInternalDependencies": "patch",
9-
"ignore": ["docs", "example-*"]
9+
"ignore": ["docs", "example-*", "codesandbox"]
1010
}

‎packages/react/src/Banner/Banner.docs.json

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@
66
"importPath": "@primer/react/experimental",
77
"stories": [],
88
"props": [
9+
{
10+
"name": "aria-label",
11+
"type": "string",
12+
"description": "Provide an optional label to override the default name for the Banner landmark region"
13+
},
914
{
1015
"name": "description",
1116
"type": "React.ReactNode",

‎packages/react/src/Banner/Banner.test.tsx

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,43 @@ describe('Banner', () => {
2525

2626
it('should render as a region element', () => {
2727
render(<Banner title="test" />)
28-
expect(screen.getByRole('region', {name: 'test'})).toBeInTheDocument()
28+
expect(screen.getByRole('region', {name: 'Information'})).toBeInTheDocument()
29+
expect(screen.getByRole('heading', {name: 'test'})).toBeInTheDocument()
2930
})
3031

31-
it('should label the landmark element with the title prop', () => {
32+
it('should label the landmark element with the corresponding variant label text', () => {
3233
render(<Banner title="test" />)
33-
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('test'))
34+
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information'))
35+
})
36+
37+
it('should label the landmark element with the label for the critical variant', () => {
38+
render(<Banner title="test" variant="critical" />)
39+
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Critical'))
40+
})
41+
42+
it('should label the landmark element with the label for the info variant', () => {
43+
render(<Banner title="test" variant="info" />)
44+
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information'))
45+
})
46+
47+
it('should label the landmark element with the label for the success variant', () => {
48+
render(<Banner title="test" variant="success" />)
49+
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Success'))
50+
})
51+
52+
it('should label the landmark element with the label for the upsell variant', () => {
53+
render(<Banner title="test" variant="upsell" />)
54+
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Recommendation'))
55+
})
56+
57+
it('should label the landmark element with the label for the warning variant', () => {
58+
render(<Banner title="test" variant="warning" />)
59+
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Warning'))
60+
})
61+
62+
it('should support the `aria-label` prop to override the default label for the landmark', () => {
63+
render(<Banner aria-label="Test" title="test" variant="warning" />)
64+
expect(screen.getByRole('region')).toHaveAttribute('aria-label', 'Test')
3465
})
3566

3667
it('should default the title to a h2', () => {
@@ -50,7 +81,7 @@ describe('Banner', () => {
5081
it('should rendering a description with the `description` prop', () => {
5182
render(<Banner title="test" description="test-description" />)
5283
expect(screen.getByText('test-description')).toBeInTheDocument()
53-
expect(screen.getByRole('region', {name: 'test'})).toContainElement(screen.getByText('test-description'))
84+
expect(screen.getByRole('region', {name: 'Information'})).toContainElement(screen.getByText('test-description'))
5485
})
5586

5687
it('should support a primary action', async () => {

‎packages/react/src/Banner/Banner.tsx

Lines changed: 83 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,21 @@
11
import cx from 'clsx'
2-
import React, {createContext, useContext, useEffect, useId, useMemo} from 'react'
2+
import React, {useEffect} from 'react'
33
import styled from 'styled-components'
44
import {AlertIcon, InfoIcon, StopIcon, CheckCircleIcon, XIcon} from '@primer/octicons-react'
55
import {Button, IconButton} from '../Button'
66
import {get} from '../constants'
77
import {VisuallyHidden} from '../internal/components/VisuallyHidden'
8+
import {useMergedRefs} from '../internal/hooks/useMergedRefs'
89

910
type BannerVariant = 'critical' | 'info' | 'success' | 'upsell' | 'warning'
1011

1112
export type BannerProps = React.ComponentPropsWithoutRef<'section'> & {
13+
/**
14+
* Provide an optional label to override the default name for the Banner
15+
* landmark region
16+
*/
17+
'aria-label'?: string
18+
1219
/**
1320
* Provide an optional description for the Banner. This should provide
1421
* supplemental information about the Banner
@@ -64,74 +71,96 @@ const iconForVariant: Record<BannerVariant, React.ReactNode> = {
6471
warning: <AlertIcon />,
6572
}
6673

74+
const labels: Record<BannerVariant, string> = {
75+
critical: 'Critical',
76+
info: 'Information',
77+
success: 'Success',
78+
upsell: 'Recommendation',
79+
warning: 'Warning',
80+
}
81+
6782
export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner(
68-
{children, description, hideTitle, icon, onDismiss, primaryAction, secondaryAction, title, variant = 'info', ...rest},
69-
ref,
83+
{
84+
'aria-label': label,
85+
children,
86+
description,
87+
hideTitle,
88+
icon,
89+
onDismiss,
90+
primaryAction,
91+
secondaryAction,
92+
title,
93+
variant = 'info',
94+
...rest
95+
},
96+
forwardRef,
7097
) {
71-
const titleId = useId()
72-
const value = useMemo(() => {
73-
return {
74-
titleId,
75-
}
76-
}, [titleId])
7798
const dismissible = variant !== 'critical' && onDismiss
7899
const hasActions = primaryAction || secondaryAction
100+
const bannerRef = React.useRef<HTMLElement>(null)
101+
const ref = useMergedRefs(forwardRef, bannerRef)
79102

80103
if (__DEV__) {
81-
// Note: __DEV__ will make it so that this hook is consistently called, or
82-
// not called, depending on environment
104+
// This hook is called consistently depending on the environment
83105
// eslint-disable-next-line react-hooks/rules-of-hooks
84106
useEffect(() => {
85-
const title = document.getElementById(titleId)
86-
if (!title) {
107+
if (title) {
108+
return
109+
}
110+
111+
const {current: banner} = bannerRef
112+
if (!banner) {
113+
return
114+
}
115+
116+
const hasTitle = banner.querySelector('[data-banner-title]')
117+
if (!hasTitle) {
87118
throw new Error(
88-
'The Banner component requires a title to be provided as the `title` prop or through `Banner.Title`',
119+
'Expected a title to be provided to the <Banner> component with the `title` prop or through `<Banner.Title>` but no title was found',
89120
)
90121
}
91-
}, [titleId])
122+
}, [title])
92123
}
93124

94125
return (
95-
<BannerContext.Provider value={value}>
96-
<StyledBanner
97-
{...rest}
98-
aria-labelledby={titleId}
99-
as="section"
100-
data-dismissible={onDismiss ? '' : undefined}
101-
data-title-hidden={hideTitle ? '' : undefined}
102-
data-variant={variant}
103-
tabIndex={-1}
104-
ref={ref}
105-
>
106-
<style>{BannerContainerQuery}</style>
107-
<div className="BannerIcon">{icon && variant === 'info' ? icon : iconForVariant[variant]}</div>
108-
<div className="BannerContainer">
109-
<div className="BannerContent">
110-
{title ? (
111-
hideTitle ? (
112-
<VisuallyHidden>
113-
<BannerTitle>{title}</BannerTitle>
114-
</VisuallyHidden>
115-
) : (
126+
<StyledBanner
127+
{...rest}
128+
aria-label={label ?? labels[variant]}
129+
as="section"
130+
data-dismissible={onDismiss ? '' : undefined}
131+
data-title-hidden={hideTitle ? '' : undefined}
132+
data-variant={variant}
133+
tabIndex={-1}
134+
ref={ref}
135+
>
136+
<style>{BannerContainerQuery}</style>
137+
<div className="BannerIcon">{icon && variant === 'info' ? icon : iconForVariant[variant]}</div>
138+
<div className="BannerContainer">
139+
<div className="BannerContent">
140+
{title ? (
141+
hideTitle ? (
142+
<VisuallyHidden>
116143
<BannerTitle>{title}</BannerTitle>
117-
)
118-
) : null}
119-
{description ? <BannerDescription>{description}</BannerDescription> : null}
120-
{children}
121-
</div>
122-
{hasActions ? <BannerActions primaryAction={primaryAction} secondaryAction={secondaryAction} /> : null}
144+
</VisuallyHidden>
145+
) : (
146+
<BannerTitle>{title}</BannerTitle>
147+
)
148+
) : null}
149+
{description ? <BannerDescription>{description}</BannerDescription> : null}
150+
{children}
123151
</div>
124-
{dismissible ? (
125-
<IconButton
126-
aria-label="Dismiss banner"
127-
onClick={onDismiss}
128-
className="BannerDismiss"
129-
icon={XIcon}
130-
variant="invisible"
131-
/>
132-
) : null}
133-
</StyledBanner>
134-
</BannerContext.Provider>
152+
{hasActions ? <BannerActions primaryAction={primaryAction} secondaryAction={secondaryAction} /> : null}
153+
</div>
154+
{dismissible ? (
155+
<IconButton
156+
aria-label="Dismiss banner"
157+
onClick={onDismiss}
158+
className="BannerDismiss"
159+
icon={XIcon}
160+
variant="invisible"
161+
/>
162+
) : null}
163+
</StyledBanner>
135164
)
136165
})
137166

@@ -342,9 +371,8 @@ export type BannerTitleProps<As extends HeadingElement> = {
342371

343372
export function BannerTitle<As extends HeadingElement>(props: BannerTitleProps<As>) {
344373
const {as: Heading = 'h2', className, children, ...rest} = props
345-
const banner = useBanner()
346374
return (
347-
<Heading {...rest} id={banner.titleId} className={cx('BannerTitle', className)}>
375+
<Heading {...rest} className={cx('BannerTitle', className)} data-banner-title="">
348376
{children}
349377
</Heading>
350378
)
@@ -399,14 +427,3 @@ export function BannerSecondaryAction({children, className, ...rest}: BannerSeco
399427
</Button>
400428
)
401429
}
402-
403-
type BannerContextValue = {titleId: string}
404-
const BannerContext = createContext<BannerContextValue | null>(null)
405-
406-
function useBanner(): BannerContextValue {
407-
const value = useContext(BannerContext)
408-
if (value) {
409-
return value
410-
}
411-
throw new Error('Component must be used within a <Banner> component')
412-
}

‎packages/react/src/Banner/__snapshots__/Banner.test.tsx.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
// Jest Snapshot v1, https://goo.gl/fbAQLP
22

3-
exports[`Banner should throw an error if no title is provided 1`] = `"The Banner component requires a title to be provided as the \`title\` prop or through \`Banner.Title\`"`;
3+
exports[`Banner should throw an error if no title is provided 1`] = `"Expected a title to be provided to the <Banner> component with the \`title\` prop or through \`<Banner.Title>\` but no title was found"`;

‎packages/react/src/internal/hooks/useMergedRefs.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import {useCallback} from 'react'
2+
3+
export function useMergedRefs<T>(
4+
...refs: Array<React.MutableRefObject<T> | React.ForwardedRef<T> | React.RefCallback<T>>
5+
): React.RefCallback<T> {
6+
return useCallback((instance: T) => {
7+
for (const ref of refs) {
8+
if (typeof ref === 'function') {
9+
ref(instance)
10+
} else if (ref) {
11+
ref.current = instance
12+
}
13+
}
14+
// eslint-disable-next-line react-hooks/exhaustive-deps
15+
}, refs)
16+
}

0 commit comments

Comments
 (0)