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 <16447748+khiga8@users.noreply.github.com> * Update packages/react/src/Banner/Banner.test.tsx Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> * test: update test label with aria-label --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
primer · May 3, 2024 · f0de234 · f0de234
1 parent 9a56727
commit f0de234
Show file tree

Hide file tree

Showing 7 changed files with 146 additions and 72 deletions.
diff --git a/.changeset/cold-starfishes-shout.md 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
@@ -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
@@ -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
@@ -25,12 +25,43 @@ describe('Banner', () => {
 
   it('should render as a region element', () => {
     render(<Banner title="test" />)
-    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(<Banner title="test" />)
-    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(<Banner title="test" variant="critical" />)
+    expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Critical'))
+  })
+
+  it('should label the landmark element with the label for the info variant', () => {
+    render(<Banner title="test" variant="info" />)
+    expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Information'))
+  })
+
+  it('should label the landmark element with the label for the success variant', () => {
+    render(<Banner title="test" variant="success" />)
+    expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Success'))
+  })
+
+  it('should label the landmark element with the label for the upsell variant', () => {
+    render(<Banner title="test" variant="upsell" />)
+    expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Recommendation'))
+  })
+
+  it('should label the landmark element with the label for the warning variant', () => {
+    render(<Banner title="test" variant="warning" />)
+    expect(screen.getByRole('region')).toEqual(screen.getByLabelText('Warning'))
+  })
+
+  it('should support the `aria-label` prop to override the default label for the landmark', () => {
+    render(<Banner aria-label="Test" title="test" variant="warning" />)
+    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(<Banner title="test" description="test-description" />)
     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
@@ -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<BannerVariant, React.ReactNode> = {
   warning: <AlertIcon />,
 }
 
+const labels: Record<BannerVariant, string> = {
+  critical: 'Critical',
+  info: 'Information',
+  success: 'Success',
+  upsell: 'Recommendation',
+  warning: 'Warning',
+}
+
 export const Banner = React.forwardRef<HTMLElement, BannerProps>(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<HTMLElement>(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 <Banner> component with the `title` prop or through `<Banner.Title>` but no title was found',
         )
       }
-    }, [titleId])
+    }, [title])
   }
 
   return (
-    <BannerContext.Provider value={value}>
-      <StyledBanner
-        {...rest}
-        aria-labelledby={titleId}
-        as="section"
-        data-dismissible={onDismiss ? '' : undefined}
-        data-title-hidden={hideTitle ? '' : undefined}
-        data-variant={variant}
-        tabIndex={-1}
-        ref={ref}
-      >
-        <style>{BannerContainerQuery}</style>
-        <div className="BannerIcon">{icon && variant === 'info' ? icon : iconForVariant[variant]}</div>
-        <div className="BannerContainer">
-          <div className="BannerContent">
-            {title ? (
-              hideTitle ? (
-                <VisuallyHidden>
-                  <BannerTitle>{title}</BannerTitle>
-                </VisuallyHidden>
-              ) : (
+    <StyledBanner
+      {...rest}
+      aria-label={label ?? labels[variant]}
+      as="section"
+      data-dismissible={onDismiss ? '' : undefined}
+      data-title-hidden={hideTitle ? '' : undefined}
+      data-variant={variant}
+      tabIndex={-1}
+      ref={ref}
+    >
+      <style>{BannerContainerQuery}</style>
+      <div className="BannerIcon">{icon && variant === 'info' ? icon : iconForVariant[variant]}</div>
+      <div className="BannerContainer">
+        <div className="BannerContent">
+          {title ? (
+            hideTitle ? (
+              <VisuallyHidden>
                 <BannerTitle>{title}</BannerTitle>
-              )
-            ) : null}
-            {description ? <BannerDescription>{description}</BannerDescription> : null}
-            {children}
-          </div>
-          {hasActions ? <BannerActions primaryAction={primaryAction} secondaryAction={secondaryAction} /> : null}
+              </VisuallyHidden>
+            ) : (
+              <BannerTitle>{title}</BannerTitle>
+            )
+          ) : null}
+          {description ? <BannerDescription>{description}</BannerDescription> : null}
+          {children}
         </div>
-        {dismissible ? (
-          <IconButton
-            aria-label="Dismiss banner"
-            onClick={onDismiss}
-            className="BannerDismiss"
-            icon={XIcon}
-            variant="invisible"
-          />
-        ) : null}
-      </StyledBanner>
-    </BannerContext.Provider>
+        {hasActions ? <BannerActions primaryAction={primaryAction} secondaryAction={secondaryAction} /> : null}
+      </div>
+      {dismissible ? (
+        <IconButton
+          aria-label="Dismiss banner"
+          onClick={onDismiss}
+          className="BannerDismiss"
+          icon={XIcon}
+          variant="invisible"
+        />
+      ) : null}
+    </StyledBanner>
   )
 })
 
@@ -342,9 +371,8 @@ export type BannerTitleProps<As extends HeadingElement> = {
 
 export function BannerTitle<As extends HeadingElement>(props: BannerTitleProps<As>) {
   const {as: Heading = 'h2', className, children, ...rest} = props
-  const banner = useBanner()
   return (
-    <Heading {...rest} id={banner.titleId} className={cx('BannerTitle', className)}>
+    <Heading {...rest} className={cx('BannerTitle', className)} data-banner-title="">
       {children}
     </Heading>
   )
@@ -399,14 +427,3 @@ export function BannerSecondaryAction({children, className, ...rest}: BannerSeco
     </Button>
   )
 }
-
-type BannerContextValue = {titleId: string}
-const BannerContext = createContext<BannerContextValue | null>(null)
-
-function useBanner(): BannerContextValue {
-  const value = useContext(BannerContext)
-  if (value) {
-    return value
-  }
-  throw new Error('Component must be used within a <Banner> component')
-}
diff --git 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 <Banner> component with the \`title\` prop or through \`<Banner.Title>\` but no title was found"`;
diff --git a/packages/react/src/internal/hooks/useMergedRefs.ts b/packages/react/src/internal/hooks/useMergedRefs.ts
@@ -0,0 +1,16 @@
+import {useCallback} from 'react'
+
+export function useMergedRefs<T>(
+  ...refs: Array<React.MutableRefObject<T> | React.ForwardedRef<T> | React.RefCallback<T>>
+): React.RefCallback<T> {
+  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)
+}