diff --git a/.changeset/cold-starfishes-shout.md b/.changeset/cold-starfishes-shout.md new file mode 100644 index 00000000000..ce8c32e1b71 --- /dev/null +++ b/.changeset/cold-starfishes-shout.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +Update Banner to use an explicit aria-label instead of being labelled by Banner title diff --git a/.changeset/config.json b/.changeset/config.json index 7c4d724582b..d826f57cc65 100644 --- a/.changeset/config.json +++ b/.changeset/config.json @@ -6,5 +6,5 @@ "access": "public", "baseBranch": "main", "updateInternalDependencies": "patch", - "ignore": ["docs", "example-*"] + "ignore": ["docs", "example-*", "codesandbox"] } diff --git a/packages/react/src/Banner/Banner.docs.json b/packages/react/src/Banner/Banner.docs.json index a12ab01b984..fa4570ec9b0 100644 --- a/packages/react/src/Banner/Banner.docs.json +++ b/packages/react/src/Banner/Banner.docs.json @@ -6,6 +6,11 @@ "importPath": "@primer/react/experimental", "stories": [], "props": [ + { + "name": "aria-label", + "type": "string", + "description": "Provide an optional label to override the default name for the Banner landmark region" + }, { "name": "description", "type": "React.ReactNode", diff --git a/packages/react/src/Banner/Banner.test.tsx b/packages/react/src/Banner/Banner.test.tsx index 0da386c115e..0664cc0c692 100644 --- a/packages/react/src/Banner/Banner.test.tsx +++ b/packages/react/src/Banner/Banner.test.tsx @@ -25,12 +25,43 @@ describe('Banner', () => { it('should render as a region element', () => { render() - expect(screen.getByRole('region', {name: 'test'})).toBeInTheDocument() + expect(screen.getByRole('region', {name: 'Information'})).toBeInTheDocument() + expect(screen.getByRole('heading', {name: 'test'})).toBeInTheDocument() }) - it('should label the landmark element with the title prop', () => { + it('should label the landmark element with the corresponding variant label text', () => { render() - expect(screen.getByRole('region')).toEqual(screen.getByLabelText('test')) + expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information')) + }) + + it('should label the landmark element with the label for the critical variant', () => { + render() + expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Critical')) + }) + + it('should label the landmark element with the label for the info variant', () => { + render() + expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information')) + }) + + it('should label the landmark element with the label for the success variant', () => { + render() + expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Success')) + }) + + it('should label the landmark element with the label for the upsell variant', () => { + render() + expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Recommendation')) + }) + + it('should label the landmark element with the label for the warning variant', () => { + render() + expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Warning')) + }) + + it('should support the `aria-label` prop to override the default label for the landmark', () => { + render() + expect(screen.getByRole('region')).toHaveAttribute('aria-label', 'Test') }) it('should default the title to a h2', () => { @@ -50,7 +81,7 @@ describe('Banner', () => { it('should rendering a description with the `description` prop', () => { render() expect(screen.getByText('test-description')).toBeInTheDocument() - expect(screen.getByRole('region', {name: 'test'})).toContainElement(screen.getByText('test-description')) + expect(screen.getByRole('region', {name: 'Information'})).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 13fb6196dde..8f61ba1a1d1 100644 --- a/packages/react/src/Banner/Banner.tsx +++ b/packages/react/src/Banner/Banner.tsx @@ -1,14 +1,21 @@ import cx from 'clsx' -import React, {createContext, useContext, useEffect, useId, useMemo} from 'react' +import React, {useEffect} from 'react' import styled from 'styled-components' import {AlertIcon, InfoIcon, StopIcon, CheckCircleIcon, XIcon} from '@primer/octicons-react' import {Button, IconButton} from '../Button' import {get} from '../constants' import {VisuallyHidden} from '../internal/components/VisuallyHidden' +import {useMergedRefs} from '../internal/hooks/useMergedRefs' type BannerVariant = 'critical' | 'info' | 'success' | 'upsell' | 'warning' export type BannerProps = React.ComponentPropsWithoutRef<'section'> & { + /** + * Provide an optional label to override the default name for the Banner + * landmark region + */ + 'aria-label'?: string + /** * Provide an optional description for the Banner. This should provide * supplemental information about the Banner @@ -64,74 +71,96 @@ const iconForVariant: Record = { warning: , } +const labels: Record = { + critical: 'Critical', + info: 'Information', + success: 'Success', + upsell: 'Recommendation', + warning: 'Warning', +} + export const Banner = React.forwardRef(function Banner( - {children, description, hideTitle, icon, onDismiss, primaryAction, secondaryAction, title, variant = 'info', ...rest}, - ref, + { + 'aria-label': label, + children, + description, + hideTitle, + icon, + onDismiss, + primaryAction, + secondaryAction, + title, + variant = 'info', + ...rest + }, + forwardRef, ) { - const titleId = useId() - const value = useMemo(() => { - return { - titleId, - } - }, [titleId]) const dismissible = variant !== 'critical' && onDismiss const hasActions = primaryAction || secondaryAction + const bannerRef = React.useRef(null) + const ref = useMergedRefs(forwardRef, bannerRef) if (__DEV__) { - // Note: __DEV__ will make it so that this hook is consistently called, or - // not called, depending on environment + // This hook is called consistently depending on the environment // eslint-disable-next-line react-hooks/rules-of-hooks useEffect(() => { - const title = document.getElementById(titleId) - if (!title) { + if (title) { + return + } + + const {current: banner} = bannerRef + if (!banner) { + return + } + + const hasTitle = banner.querySelector('[data-banner-title]') + if (!hasTitle) { throw new Error( - 'The Banner component requires a title to be provided as the `title` prop or through `Banner.Title`', + 'Expected a title to be provided to the component with the `title` prop or through `` but no title was found', ) } - }, [titleId]) + }, [title]) } return ( - - - -
{icon && variant === 'info' ? icon : iconForVariant[variant]}
-
-
- {title ? ( - hideTitle ? ( - - {title} - - ) : ( + + +
{icon && variant === 'info' ? icon : iconForVariant[variant]}
+
+
+ {title ? ( + hideTitle ? ( + {title} - ) - ) : null} - {description ? {description} : null} - {children} -
- {hasActions ? : null} + + ) : ( + {title} + ) + ) : null} + {description ? {description} : null} + {children}
- {dismissible ? ( - - ) : null} -
- + {hasActions ? : null} +
+ {dismissible ? ( + + ) : null} + ) }) @@ -342,9 +371,8 @@ export type BannerTitleProps = { export function BannerTitle(props: BannerTitleProps) { const {as: Heading = 'h2', className, children, ...rest} = props - const banner = useBanner() return ( - + {children} ) @@ -399,14 +427,3 @@ export function BannerSecondaryAction({children, className, ...rest}: BannerSeco ) } - -type BannerContextValue = {titleId: string} -const BannerContext = createContext(null) - -function useBanner(): BannerContextValue { - const value = useContext(BannerContext) - if (value) { - return value - } - throw new Error('Component must be used within a component') -} diff --git a/packages/react/src/Banner/__snapshots__/Banner.test.tsx.snap b/packages/react/src/Banner/__snapshots__/Banner.test.tsx.snap index 3a1a9768fbc..34992a57706 100644 --- a/packages/react/src/Banner/__snapshots__/Banner.test.tsx.snap +++ b/packages/react/src/Banner/__snapshots__/Banner.test.tsx.snap @@ -1,3 +1,3 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -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\`"`; +exports[`Banner should throw an error if no title is provided 1`] = `"Expected a title to be provided to the component with the \`title\` prop or through \`\` but no title was found"`; diff --git a/packages/react/src/internal/hooks/useMergedRefs.ts b/packages/react/src/internal/hooks/useMergedRefs.ts new file mode 100644 index 00000000000..7c67f20fadc --- /dev/null +++ b/packages/react/src/internal/hooks/useMergedRefs.ts @@ -0,0 +1,16 @@ +import {useCallback} from 'react' + +export function useMergedRefs( + ...refs: Array | React.ForwardedRef | React.RefCallback> +): React.RefCallback { + return useCallback((instance: T) => { + for (const ref of refs) { + if (typeof ref === 'function') { + ref(instance) + } else if (ref) { + ref.current = instance + } + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, refs) +}