Skip to content

Commit 9658737

Browse files
Copilotlangermank
Copilot
and
langermank
committed
Update Banner to use aria-labelledby with Banner.Title by default
Co-authored-by: langermank <18661030+langermank@users.noreply.github.com>
1 parent 29b6c6c commit 9658737

File tree

2 files changed

+104
-68
lines changed

2 files changed

+104
-68
lines changed

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

Lines changed: 51 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {Banner} from '../Banner'
66
describe('Banner', () => {
77
it('should render as a region element', () => {
88
render(<Banner title="test" />)
9-
expect(screen.getByRole('region', {name: 'Information'})).toBeInTheDocument()
9+
expect(screen.getByRole('region', {name: 'test'})).toBeInTheDocument()
1010
expect(screen.getByRole('heading', {name: 'test'})).toBeInTheDocument()
1111
})
1212

@@ -15,34 +15,41 @@ describe('Banner', () => {
1515
expect(render(<Element />).container.firstChild).toHaveClass('test-class-name')
1616
})
1717

18-
it('should label the landmark element with the corresponding variant label text', () => {
19-
render(<Banner title="test" />)
20-
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information'))
18+
it('should label the landmark element with the title by default', () => {
19+
render(<Banner title="My Banner Title" />)
20+
const region = screen.getByRole('region', {name: 'My Banner Title'})
21+
expect(region).toHaveAttribute('aria-labelledby')
22+
expect(region).not.toHaveAttribute('aria-label')
2123
})
2224

23-
it('should label the landmark element with the label for the critical variant', () => {
24-
render(<Banner title="test" variant="critical" />)
25-
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Critical'))
25+
it('should use aria-labelledby to reference the title for the critical variant', () => {
26+
render(<Banner title="Critical Issue" variant="critical" />)
27+
const region = screen.getByRole('region', {name: 'Critical Issue'})
28+
expect(region).toHaveAttribute('aria-labelledby')
2629
})
2730

28-
it('should label the landmark element with the label for the info variant', () => {
29-
render(<Banner title="test" variant="info" />)
30-
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information'))
31+
it('should use aria-labelledby to reference the title for the info variant', () => {
32+
render(<Banner title="Information" variant="info" />)
33+
const region = screen.getByRole('region', {name: 'Information'})
34+
expect(region).toHaveAttribute('aria-labelledby')
3135
})
3236

33-
it('should label the landmark element with the label for the success variant', () => {
34-
render(<Banner title="test" variant="success" />)
35-
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Success'))
37+
it('should use aria-labelledby to reference the title for the success variant', () => {
38+
render(<Banner title="Success Message" variant="success" />)
39+
const region = screen.getByRole('region', {name: 'Success Message'})
40+
expect(region).toHaveAttribute('aria-labelledby')
3641
})
3742

38-
it('should label the landmark element with the label for the upsell variant', () => {
39-
render(<Banner title="test" variant="upsell" />)
40-
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Recommendation'))
43+
it('should use aria-labelledby to reference the title for the upsell variant', () => {
44+
render(<Banner title="Recommendation" variant="upsell" />)
45+
const region = screen.getByRole('region', {name: 'Recommendation'})
46+
expect(region).toHaveAttribute('aria-labelledby')
4147
})
4248

43-
it('should label the landmark element with the label for the warning variant', () => {
44-
render(<Banner title="test" variant="warning" />)
45-
expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Warning'))
49+
it('should use aria-labelledby to reference the title for the warning variant', () => {
50+
render(<Banner title="Warning" variant="warning" />)
51+
const region = screen.getByRole('region', {name: 'Warning'})
52+
expect(region).toHaveAttribute('aria-labelledby')
4653
})
4754

4855
it('should support the `aria-label` prop to override the default label for the landmark', () => {
@@ -69,6 +76,30 @@ describe('Banner', () => {
6976
expect(region).not.toHaveAttribute('aria-labelledby')
7077
})
7178

79+
it('should use aria-labelledby to reference Banner.Title when provided as a child', () => {
80+
render(
81+
<Banner>
82+
<Banner.Title>Custom Title Component</Banner.Title>
83+
</Banner>,
84+
)
85+
const region = screen.getByRole('region', {name: 'Custom Title Component'})
86+
const heading = screen.getByRole('heading', {name: 'Custom Title Component'})
87+
expect(region).toHaveAttribute('aria-labelledby', heading.id)
88+
expect(heading).toHaveAttribute('id')
89+
expect(region).not.toHaveAttribute('aria-label')
90+
})
91+
92+
it('should use aria-labelledby to reference Banner.Title with custom id', () => {
93+
render(
94+
<Banner aria-labelledby="custom-title-id">
95+
<Banner.Title id="custom-title-id">Title with Custom ID</Banner.Title>
96+
</Banner>,
97+
)
98+
const region = screen.getByRole('region', {name: 'Title with Custom ID'})
99+
expect(region).toHaveAttribute('aria-labelledby', 'custom-title-id')
100+
expect(screen.getByRole('heading')).toHaveAttribute('id', 'custom-title-id')
101+
})
102+
72103
it('should default the title to a h2', () => {
73104
render(<Banner title="test" />)
74105
expect(screen.getByRole('heading', {level: 2})).toBeInTheDocument()
@@ -86,7 +117,7 @@ describe('Banner', () => {
86117
it('should rendering a description with the `description` prop', () => {
87118
render(<Banner title="test" description="test-description" />)
88119
expect(screen.getByText('test-description')).toBeInTheDocument()
89-
expect(screen.getByRole('region', {name: 'Information'})).toContainElement(screen.getByText('test-description'))
120+
expect(screen.getByRole('region', {name: 'test'})).toContainElement(screen.getByText('test-description'))
90121
})
91122

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

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

Lines changed: 53 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,18 @@ import {AlertIcon, InfoIcon, StopIcon, CheckCircleIcon, XIcon} from '@primer/oct
44
import {Button, IconButton, type ButtonProps} from '../Button'
55
import {VisuallyHidden} from '../VisuallyHidden'
66
import {useMergedRefs} from '../internal/hooks/useMergedRefs'
7+
import {useId} from '../hooks/useId'
78
import classes from './Banner.module.css'
89
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
910

1011
export type BannerVariant = 'critical' | 'info' | 'success' | 'upsell' | 'warning'
1112

13+
type BannerContextValue = {
14+
titleId: string
15+
}
16+
17+
const BannerContext = React.createContext<BannerContextValue | undefined>(undefined)
18+
1219
export type BannerProps = React.ComponentPropsWithoutRef<'section'> & {
1320
/**
1421
* Provide an optional label to override the default name for the Banner
@@ -84,14 +91,6 @@ const iconForVariant: Record<BannerVariant, React.ReactNode> = {
8491
warning: <AlertIcon />,
8592
}
8693

87-
const labels: Record<BannerVariant, string> = {
88-
critical: 'Critical',
89-
info: 'Information',
90-
success: 'Success',
91-
upsell: 'Recommendation',
92-
warning: 'Warning',
93-
}
94-
9594
export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner(
9695
{
9796
'aria-label': label,
@@ -116,6 +115,7 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
116115
const bannerRef = React.useRef<HTMLElement>(null)
117116
const ref = useMergedRefs(forwardRef, bannerRef)
118117
const supportsCustomIcon = variant === 'info' || variant === 'upsell'
118+
const titleId = useId()
119119

120120
if (__DEV__) {
121121
// This hook is called consistently depending on the environment
@@ -141,46 +141,48 @@ export const Banner = React.forwardRef<HTMLElement, BannerProps>(function Banner
141141
}
142142

143143
return (
144-
<section
145-
{...rest}
146-
aria-labelledby={labelledBy}
147-
aria-label={labelledBy ? undefined : (label ?? labels[variant])}
148-
className={clsx(className, classes.Banner)}
149-
data-dismissible={onDismiss ? '' : undefined}
150-
data-title-hidden={hideTitle ? '' : undefined}
151-
data-variant={variant}
152-
data-actions-layout={actionsLayout}
153-
tabIndex={-1}
154-
ref={ref}
155-
data-layout={rest.layout || 'default'}
156-
>
157-
<div className={classes.BannerIcon}>{icon && supportsCustomIcon ? icon : iconForVariant[variant]}</div>
158-
<div className={classes.BannerContainer}>
159-
<div className={classes.BannerContent}>
160-
{title ? (
161-
hideTitle ? (
162-
<VisuallyHidden>
144+
<BannerContext.Provider value={{titleId}}>
145+
<section
146+
{...rest}
147+
aria-labelledby={labelledBy ?? (label ? undefined : titleId)}
148+
aria-label={labelledBy ? undefined : label}
149+
className={clsx(className, classes.Banner)}
150+
data-dismissible={onDismiss ? '' : undefined}
151+
data-title-hidden={hideTitle ? '' : undefined}
152+
data-variant={variant}
153+
data-actions-layout={actionsLayout}
154+
tabIndex={-1}
155+
ref={ref}
156+
data-layout={rest.layout || 'default'}
157+
>
158+
<div className={classes.BannerIcon}>{icon && supportsCustomIcon ? icon : iconForVariant[variant]}</div>
159+
<div className={classes.BannerContainer}>
160+
<div className={classes.BannerContent}>
161+
{title ? (
162+
hideTitle ? (
163+
<VisuallyHidden>
164+
<BannerTitle>{title}</BannerTitle>
165+
</VisuallyHidden>
166+
) : (
163167
<BannerTitle>{title}</BannerTitle>
164-
</VisuallyHidden>
165-
) : (
166-
<BannerTitle>{title}</BannerTitle>
167-
)
168-
) : null}
169-
{description ? <BannerDescription>{description}</BannerDescription> : null}
170-
{children}
168+
)
169+
) : null}
170+
{description ? <BannerDescription>{description}</BannerDescription> : null}
171+
{children}
172+
</div>
173+
{hasActions ? <BannerActions primaryAction={primaryAction} secondaryAction={secondaryAction} /> : null}
171174
</div>
172-
{hasActions ? <BannerActions primaryAction={primaryAction} secondaryAction={secondaryAction} /> : null}
173-
</div>
174-
{dismissible ? (
175-
<IconButton
176-
aria-label="Dismiss banner"
177-
onClick={onDismiss}
178-
className={classes.BannerDismiss}
179-
icon={XIcon}
180-
variant="invisible"
181-
/>
182-
) : null}
183-
</section>
175+
{dismissible ? (
176+
<IconButton
177+
aria-label="Dismiss banner"
178+
onClick={onDismiss}
179+
className={classes.BannerDismiss}
180+
icon={XIcon}
181+
variant="invisible"
182+
/>
183+
) : null}
184+
</section>
185+
</BannerContext.Provider>
184186
)
185187
})
186188

@@ -192,9 +194,12 @@ export type BannerTitleProps<As extends HeadingElement> = {
192194
} & React.ComponentPropsWithoutRef<As extends 'h2' ? 'h2' : As>
193195

194196
export function BannerTitle<As extends HeadingElement>(props: BannerTitleProps<As>) {
195-
const {as: Heading = 'h2', className, children, ...rest} = props
197+
const {as: Heading = 'h2', className, children, id, ...rest} = props
198+
const context = React.useContext(BannerContext)
199+
const titleId = id ?? context?.titleId
200+
196201
return (
197-
<Heading {...rest} className={clsx(className, classes.BannerTitle)} data-banner-title="">
202+
<Heading {...rest} id={titleId} className={clsx(className, classes.BannerTitle)} data-banner-title="">
198203
{children}
199204
</Heading>
200205
)

0 commit comments

Comments
 (0)