From 29b6c6ccde0dc324e8846a972c2ed521bb0bd088 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Oct 2025 23:07:59 +0000 Subject: [PATCH 1/2] Initial plan From 9658737422fc171186ebd20cced91b2c3d2eb678 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 29 Oct 2025 23:25:59 +0000 Subject: [PATCH 2/2] Update Banner to use aria-labelledby with Banner.Title by default Co-authored-by: langermank <18661030+langermank@users.noreply.github.com> --- packages/react/src/Banner/Banner.test.tsx | 71 ++++++++++----- packages/react/src/Banner/Banner.tsx | 101 ++++++++++++---------- 2 files changed, 104 insertions(+), 68 deletions(-) diff --git a/packages/react/src/Banner/Banner.test.tsx b/packages/react/src/Banner/Banner.test.tsx index b08b0d52bc0..d62813884f0 100644 --- a/packages/react/src/Banner/Banner.test.tsx +++ b/packages/react/src/Banner/Banner.test.tsx @@ -6,7 +6,7 @@ import {Banner} from '../Banner' describe('Banner', () => { it('should render as a region element', () => { render() - expect(screen.getByRole('region', {name: 'Information'})).toBeInTheDocument() + expect(screen.getByRole('region', {name: 'test'})).toBeInTheDocument() expect(screen.getByRole('heading', {name: 'test'})).toBeInTheDocument() }) @@ -15,34 +15,41 @@ describe('Banner', () => { expect(render().container.firstChild).toHaveClass('test-class-name') }) - it('should label the landmark element with the corresponding variant label text', () => { - render() - expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information')) + it('should label the landmark element with the title by default', () => { + render() + const region = screen.getByRole('region', {name: 'My Banner Title'}) + expect(region).toHaveAttribute('aria-labelledby') + expect(region).not.toHaveAttribute('aria-label') }) - it('should label the landmark element with the label for the critical variant', () => { - render() - expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Critical')) + it('should use aria-labelledby to reference the title for the critical variant', () => { + render() + const region = screen.getByRole('region', {name: 'Critical Issue'}) + expect(region).toHaveAttribute('aria-labelledby') }) - it('should label the landmark element with the label for the info variant', () => { - render() - expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information')) + it('should use aria-labelledby to reference the title for the info variant', () => { + render() + const region = screen.getByRole('region', {name: 'Information'}) + expect(region).toHaveAttribute('aria-labelledby') }) - it('should label the landmark element with the label for the success variant', () => { - render() - expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Success')) + it('should use aria-labelledby to reference the title for the success variant', () => { + render() + const region = screen.getByRole('region', {name: 'Success Message'}) + expect(region).toHaveAttribute('aria-labelledby') }) - it('should label the landmark element with the label for the upsell variant', () => { - render() - expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Recommendation')) + it('should use aria-labelledby to reference the title for the upsell variant', () => { + render() + const region = screen.getByRole('region', {name: 'Recommendation'}) + expect(region).toHaveAttribute('aria-labelledby') }) - it('should label the landmark element with the label for the warning variant', () => { - render() - expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Warning')) + it('should use aria-labelledby to reference the title for the warning variant', () => { + render() + const region = screen.getByRole('region', {name: 'Warning'}) + expect(region).toHaveAttribute('aria-labelledby') }) it('should support the `aria-label` prop to override the default label for the landmark', () => { @@ -69,6 +76,30 @@ describe('Banner', () => { expect(region).not.toHaveAttribute('aria-labelledby') }) + it('should use aria-labelledby to reference Banner.Title when provided as a child', () => { + render( + + Custom Title Component + , + ) + const region = screen.getByRole('region', {name: 'Custom Title Component'}) + const heading = screen.getByRole('heading', {name: 'Custom Title Component'}) + expect(region).toHaveAttribute('aria-labelledby', heading.id) + expect(heading).toHaveAttribute('id') + expect(region).not.toHaveAttribute('aria-label') + }) + + it('should use aria-labelledby to reference Banner.Title with custom id', () => { + render( + + Title with Custom ID + , + ) + const region = screen.getByRole('region', {name: 'Title with Custom ID'}) + expect(region).toHaveAttribute('aria-labelledby', 'custom-title-id') + expect(screen.getByRole('heading')).toHaveAttribute('id', 'custom-title-id') + }) + it('should default the title to a h2', () => { render() expect(screen.getByRole('heading', {level: 2})).toBeInTheDocument() @@ -86,7 +117,7 @@ describe('Banner', () => { it('should rendering a description with the `description` prop', () => { render() expect(screen.getByText('test-description')).toBeInTheDocument() - expect(screen.getByRole('region', {name: 'Information'})).toContainElement(screen.getByText('test-description')) + expect(screen.getByRole('region', {name: 'test'})).toContainElement(screen.getByText('test-description')) }) it('should support a primary action', async () => { diff --git a/packages/react/src/Banner/Banner.tsx b/packages/react/src/Banner/Banner.tsx index 81d04875cf6..31c0d6d5a47 100644 --- a/packages/react/src/Banner/Banner.tsx +++ b/packages/react/src/Banner/Banner.tsx @@ -4,11 +4,18 @@ import {AlertIcon, InfoIcon, StopIcon, CheckCircleIcon, XIcon} from '@primer/oct import {Button, IconButton, type ButtonProps} from '../Button' import {VisuallyHidden} from '../VisuallyHidden' import {useMergedRefs} from '../internal/hooks/useMergedRefs' +import {useId} from '../hooks/useId' import classes from './Banner.module.css' import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' export type BannerVariant = 'critical' | 'info' | 'success' | 'upsell' | 'warning' +type BannerContextValue = { + titleId: string +} + +const BannerContext = React.createContext(undefined) + export type BannerProps = React.ComponentPropsWithoutRef<'section'> & { /** * Provide an optional label to override the default name for the Banner @@ -84,14 +91,6 @@ const iconForVariant: Record = { warning: , } -const labels: Record = { - critical: 'Critical', - info: 'Information', - success: 'Success', - upsell: 'Recommendation', - warning: 'Warning', -} - export const Banner = React.forwardRef(function Banner( { 'aria-label': label, @@ -116,6 +115,7 @@ export const Banner = React.forwardRef(function Banner const bannerRef = React.useRef(null) const ref = useMergedRefs(forwardRef, bannerRef) const supportsCustomIcon = variant === 'info' || variant === 'upsell' + const titleId = useId() if (__DEV__) { // This hook is called consistently depending on the environment @@ -141,46 +141,48 @@ export const Banner = React.forwardRef(function Banner } return ( -
-
{icon && supportsCustomIcon ? icon : iconForVariant[variant]}
-
-
- {title ? ( - hideTitle ? ( - + +
+
{icon && supportsCustomIcon ? icon : iconForVariant[variant]}
+
+
+ {title ? ( + hideTitle ? ( + + {title} + + ) : ( {title} - - ) : ( - {title} - ) - ) : null} - {description ? {description} : null} - {children} + ) + ) : null} + {description ? {description} : null} + {children} +
+ {hasActions ? : null}
- {hasActions ? : null} -
- {dismissible ? ( - - ) : null} -
+ {dismissible ? ( + + ) : null} + + ) }) @@ -192,9 +194,12 @@ export type BannerTitleProps = { } & React.ComponentPropsWithoutRef export function BannerTitle(props: BannerTitleProps) { - const {as: Heading = 'h2', className, children, ...rest} = props + const {as: Heading = 'h2', className, children, id, ...rest} = props + const context = React.useContext(BannerContext) + const titleId = id ?? context?.titleId + return ( - + {children} )