Enforce order of child components in Card (#1048)

* simplify component type checking

* add fragment support and explicit child ordering

* add ImageAndLabelWithFragment story

* add test asserting dom order

* fix failing tests

* add changeset

* github-actions[bot] Regenerated snapshots

---------

Co-authored-by: joshfarrant <[email protected]>

Author: joshfarrant

.changeset/cyan-files-work.md

@@ -0,0 +1,5 @@
+---
+'@primer/react-brand': patch
+---
+
+Children passed to `Card` component now appear in the DOM in a predefined order to improve the experience of screen reader users.

packages/react/src/Card/Card.features.stories.tsx

@@ -226,6 +226,21 @@ export const IconAndLabel: StoryFn<typeof Card> = () => {
   )
 }
 
+export const IconAndLabelWithFragment: StoryFn<typeof Card> = () => {
+  return (
+    <Card href="https://github.com">
+      <>
+        <Card.Heading>Code search & code view</Card.Heading>
+        <Card.Label color="blue-purple">Beta</Card.Label>
+        <Card.Icon icon={ZapIcon} color="purple" hasBackground />
+        <Card.Description>
+          Enables you to rapidly search, navigate, and understand code, right from GitHub.com.
+        </Card.Description>
+      </>
+    </Card>
+  )
+}
+
 export const Image: StoryFn<typeof Card> = () => {
   return (
     <Stack direction="horizontal">

packages/react/src/Card/Card.test.tsx

@@ -31,6 +31,7 @@ describe('Card', () => {
   const mockLabel = 'This is a mock label'
   const mockHeading = 'This is a mock heading'
   const mockDescription = 'This is a mock description'
+  const mockIcon = <GitMergeIcon aria-label="Git merge icon" />
 
   it('no a11y violations', async () => {
     const {container} = render(
@@ -140,52 +141,50 @@ describe('Card', () => {
   })
 
   it('renders the label correctly into the document', () => {
-    const mockTestId = 'test'
+    const mockTestId = 'label'
     const classToCheck = 'Card__label'
 
     const {getByTestId} = render(
-      <Card href={mockHref} data-testid={mockTestId}>
-        <Card.Label>{mockLabel}</Card.Label>
+      <Card href={mockHref}>
+        <Card.Label data-testid={mockTestId}>{mockLabel}</Card.Label>
         <Card.Heading>{mockHeading}</Card.Heading>
         <Card.Description>{mockDescription}</Card.Description>
       </Card>,
     )
 
-    const cardEl = getByTestId(mockTestId).firstChild
-    expect(cardEl).toHaveClass(classToCheck)
-    expect(cardEl).toHaveTextContent(mockLabel)
+    const labelEl = getByTestId(mockTestId)
+    expect(labelEl).toHaveClass(classToCheck)
+    expect(labelEl).toHaveTextContent(mockLabel)
   })
 
   it('renders the icon correctly into the document', () => {
-    const mockTestId = 'test'
+    const mockTestId = 'icon'
     const classToCheck = 'Card__icon'
 
     const {getByTestId} = render(
-      <Card href={mockHref} data-testid={mockTestId}>
-        <Card.Icon icon={GitMergeIcon} />
+      <Card href={mockHref}>
+        <Card.Icon icon={mockIcon} data-testid={mockTestId} />
         <Card.Heading>{mockHeading}</Card.Heading>
         <Card.Description>{mockDescription}</Card.Description>
       </Card>,
     )
 
-    const cardEl = getByTestId(mockTestId).firstChild
-    expect(cardEl).toHaveClass(classToCheck)
+    expect(getByTestId(mockTestId).parentElement).toHaveClass(classToCheck)
   })
 
   it('renders the icon with background correctly into the document', () => {
-    const mockTestId = 'test'
+    const mockTestId = 'icon'
     const classToCheck = 'Icon--background'
 
     const {getByTestId} = render(
-      <Card href={mockHref} data-testid={mockTestId}>
-        <Card.Icon hasBackground icon={GitMergeIcon} />
+      <Card href={mockHref}>
+        <Card.Icon hasBackground icon={mockIcon} data-testid={mockTestId} />
         <Card.Heading>{mockHeading}</Card.Heading>
         <Card.Description>{mockDescription}</Card.Description>
       </Card>,
     )
 
-    const cardEl = getByTestId(mockTestId).firstChild
-    expect(cardEl).toHaveClass(classToCheck)
+    expect(getByTestId(mockTestId).parentElement).toHaveClass(classToCheck)
   })
 
   it('renders the image correctly into the document', () => {
@@ -246,4 +245,30 @@ describe('Card', () => {
 
     expect(cardEl.parentElement).toHaveClass('Card--fullWidth')
   })
+
+  it('renders card contents in a logical order, regardless of the order they are passed in', () => {
+    const {getByText, getByAltText, getByLabelText} = render(
+      <Card href={mockHref}>
+        <Card.Description>{mockDescription}</Card.Description>
+        <Card.Label>{mockLabel}</Card.Label>
+        <Card.Heading>{mockHeading}</Card.Heading>
+        <Card.Image src="/brand/assets/placeholder.png" alt="placeholder" />
+        <Card.Icon icon={mockIcon} />
+      </Card>,
+    )
+
+    const description = getByText(mockDescription)
+    const label = getByText(mockLabel)
+    const heading = getByText(mockHeading)
+    const image = getByAltText('placeholder')
+    const icon = getByLabelText('Git merge icon')
+
+    const isAfter = (a: Element, b: Element): boolean =>
+      a.compareDocumentPosition(b) === Node.DOCUMENT_POSITION_FOLLOWING
+
+    expect(isAfter(heading, image)).toBe(true)
+    expect(isAfter(image, icon)).toBe(true)
+    expect(isAfter(icon, label)).toBe(true)
+    expect(isAfter(label, description)).toBe(true)
+  })
 })

packages/react/src/Card/Card.tsx

@@ -76,7 +76,7 @@ const CardRoot = forwardRef<HTMLDivElement, CardProps>(
       onMouseLeave,
       onFocus,
       onBlur,
-      children,
+      children: childrenMaybeWrappedInFragment,
       className,
       ctaText = 'Learn more',
       disableAnimation = false,
@@ -92,25 +92,33 @@ const CardRoot = forwardRef<HTMLDivElement, CardProps>(
     const cardRef = useProvidedRefOrCreate(ref as RefObject<HTMLDivElement>)
     const {colorMode} = useTheme()
 
-    const filteredChildren = React.Children.toArray(children).filter(child => {
-      if (React.isValidElement(child) && typeof child.type !== 'string') {
-        if (
-          isFragment(child) ||
-          (child as React.ReactElement).type === CardImage ||
-          (child as React.ReactElement).type === CardIcon ||
-          (child as React.ReactElement).type === CardLabel ||
-          (child as React.ReactElement).type === CardHeading ||
-          (child as React.ReactElement).type === CardDescription
-        ) {
-          return true
-        }
+    const children = isFragment(childrenMaybeWrappedInFragment)
+      ? childrenMaybeWrappedInFragment.props.children
+      : childrenMaybeWrappedInFragment
+
+    const {cardImage, cardIcon, cardLabel, cardHeading, cardDescription} = React.Children.toArray(children).reduce<{
+      cardHeading?: ReturnType<typeof CardHeading>
+      cardImage?: ReturnType<typeof CardImage>
+      cardIcon?: ReturnType<typeof CardIcon>
+      cardLabel?: ReturnType<typeof CardLabel>
+      cardDescription?: ReturnType<typeof CardDescription>
+    }>((acc, child) => {
+      if (isCardHeading(child)) {
+        acc.cardHeading = React.cloneElement(child, {
+          href,
+        })
+      } else if (isCardImage(child)) {
+        acc.cardImage = child
+      } else if (isCardIcon(child)) {
+        acc.cardIcon = child
+      } else if (isCardLabel(child)) {
+        acc.cardLabel = child
+      } else if (isCardDescription(child)) {
+        acc.cardDescription = child
       }
-      return false
-    })
 
-    const hasIcon = React.Children.toArray(children).some(
-      child => React.isValidElement(child) && typeof child.type !== 'string' && child.type === CardIcon,
-    )
+      return acc
+    }, {})
 
     const hasSkewEffect = colorMode === 'dark' && variant === 'torchlight'
     const showBorder = hasSkewEffect || hasBorder
@@ -132,7 +140,7 @@ const CardRoot = forwardRef<HTMLDivElement, CardProps>(
             disableAnimation && styles['Card--disableAnimation'],
             styles[`Card--colorMode-${colorMode}`],
             styles[`Card--variant-${variant}`],
-            hasIcon && styles['Card--icon'],
+            cardIcon && styles['Card--icon'],
             showBorder && styles['Card--border'],
             styles[`Card--colorMode-${colorMode}`],
             className,
@@ -141,14 +149,12 @@ const CardRoot = forwardRef<HTMLDivElement, CardProps>(
           ref={cardRef}
           {...props}
         >
-          {React.Children.map(filteredChildren, child => {
-            if (React.isValidElement(child) && typeof child.type !== 'string' && child.type === CardHeading) {
-              return React.cloneElement<CardHeadingProps>(child as React.ReactElement<CardHeadingProps>, {
-                href,
-              })
-            }
-            return child
-          })}
+          {cardHeading}
+          {cardImage}
+          {cardIcon}
+          {cardLabel}
+          {cardDescription}
+
           <div className={styles.Card__action}>
             <Text as="span" size="200" className={clsx(stylesLink['Link--label'])}>
               {ctaText}
@@ -239,6 +245,17 @@ const CardDescription = forwardRef<HTMLParagraphElement, CardDescriptionProps>(
   },
 )
 
+const createComponentTypeGuard =
+  <T,>(componentType: React.ComponentType<T>) =>
+  (element: unknown): element is React.ReactElement<T> =>
+    React.isValidElement(element) && element.type === componentType
+
+const isCardImage = createComponentTypeGuard(CardImage)
+const isCardLabel = createComponentTypeGuard(CardLabel)
+const isCardIcon = createComponentTypeGuard(CardIcon)
+const isCardHeading = createComponentTypeGuard(CardHeading)
+const isCardDescription = createComponentTypeGuard(CardDescription)
+
 /**
  * Card component:
  * {@link https://primer.style/brand/components/Card/ See usage examples}.

packages/react/src/Card/Card.visual.spec.ts

@@ -102,6 +102,15 @@ test.describe('Visual Comparison: Card', () => {
     expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
   })
 
+  test('Card / Icon And Label With Fragment', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?args=&id=components-card-features--icon-and-label-with-fragment&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
+  })
+
   test('Card / Image', async ({page}) => {
     await page.goto('http://localhost:6006/iframe.html?args=&id=components-card-features--image&viewMode=story')
 

packages/react/src/Card/Card.visual.spec.ts-snapshots/Visual-Comparison-Card-Card-Icon-And-Label-With-Fragment-1-linux.png

@@ -200,4 +200,13 @@ test.describe('Visual Comparison: SubNav', () => {
     await page.waitForTimeout(500)
     expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
   })
+
+  test('SubNav / Keyboard Navigation', async ({page}) => {
+    await page.goto(
+      'http://localhost:6006/iframe.html?args=&id=components-subnav-features--keyboard-navigation&viewMode=story',
+    )
+
+    await page.waitForTimeout(500)
+    expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
+  })
 })