Revert #1053 and #1068, keeping FAQ.Heading bugfix

.changeset/dry-kids-shop.md

@@ -0,0 +1,5 @@
+---
+'@primer/react-brand': patch
+---
+
+Fixed a bug where an `FAQ.Heading` used within an `FAQGroup` wouldn't respect the provided `as` prop.

apps/docs/content/components/FAQ/index.mdx

@@ -70,6 +70,12 @@ When using long lists of questions and answers, consider using the FAQ group to
   </Dont>
 </DoDontContainer>
 
+## Accessibility
+
+### Heading levels
+
+The `FAQ.Heading`, `FAQ.Subheading`, `FAQ.Question`, and `FAQGroup.Heading` components all support the `as` prop, allowing the underlying HTML element to be changed. When using these components, ensure that the heading levels are semantically correct and follow a logical hierarchy within the document as the default heading levels for each component may not fit all use cases.
+
 ## Related components
 
 - [Card](/components/Card): to display navigational related content with higher visual weight.

apps/docs/content/components/FAQ/react.mdx

@@ -409,7 +409,7 @@ render(<App />)
 | `children`  | `string`                                                        |         | Sub-heading text                         |
 | `className` | `string`                                                        |         | Sub-heading custom class                 |
 | `id`        | `string`                                                        |         | Sets a custom id                         |
-| `as`        | <PropTableValues values={HeadingTags.slice(1)} addLineBreaks /> | `'h4'`  | Applies the underlying HTML element      |
+| `as`        | <PropTableValues values={HeadingTags.slice(1)} addLineBreaks /> | `'h3'`  | Applies the underlying HTML element      |
 | `ref`       | `React.RefObject`                                               |         | Forward a Ref to the underlying DOM node |
 
 <h3 id="faq-item">FAQ.Item</h3>

apps/next-docs/content/components/FAQ/index.mdx

@@ -57,6 +57,12 @@ When using long lists of questions and answers, consider using the FAQ group to
   </Dont>
 </DoDontContainer>
 
+## Accessibility
+
+### Heading levels
+
+The `FAQ.Heading`, `FAQ.Subheading`, `FAQ.Question`, and `FAQGroup.Heading` components all support the `as` prop, allowing the underlying HTML element to be changed. When using these components, ensure that the heading levels are semantically correct and follow a logical hierarchy within the document as the default heading levels for each component may not fit all use cases.
+
 ## Related components
 
 - [Card](/components/Card): to display navigational related content with higher visual weight.

apps/next-docs/content/components/FAQ/react.mdx

@@ -386,7 +386,7 @@ render(<App />)
 | `children`  | `string`                |         | Sub-heading text                         |
 | `className` | `string`                |         | Sub-heading custom class                 |
 | `id`        | `string`                |         | Sets a custom id                         |
-| `as`        | <FAQSubheadingAsProp /> | `'h4'`  | Applies the underlying HTML element      |
+| `as`        | <FAQSubheadingAsProp /> | `'h3'`  | Applies the underlying HTML element      |
 | `ref`       | `React.RefObject`       |         | Forward a Ref to the underlying DOM node |
 
 <h3 id="faq-item">FAQ.Item</h3>

packages/react/src/FAQ/FAQ.test.tsx

@@ -10,7 +10,6 @@ describe('FAQ', () => {
   const questionRoot = 'question-root'
 
   const mockHeading = 'This is a mock heading'
-  const mockSubheading = 'this is a mock subheading'
   const mockQuestion = 'What is a mock question?'
   const mockFAQAnswer = 'mock answer'
 
@@ -138,6 +137,7 @@ describe('FAQ', () => {
   })
 
   it('can render groups of FAQs using an optional sub-heading', () => {
+    const mockSubheading = 'this is a mock subheading'
     const invalidChild = <div>This is an invalid child</div>
     const {getByRole} = render(
       <FAQ>
@@ -153,12 +153,13 @@ describe('FAQ', () => {
       </FAQ>,
     )
 
-    const subheadingEl = getByRole('heading', {level: 4, name: mockSubheading})
+    const subheadingEl = getByRole('heading', {level: 3, name: mockSubheading})
 
     expect(subheadingEl).toBeInTheDocument()
   })
 
-  it('renders alternative heading levels', () => {
+  it('renders alternative headling levels', () => {
+    const mockSubheading = 'this is a mock subheading'
     const {getByRole} = render(
       <FAQ>
         <FAQ.Heading as="h3">{mockHeading}</FAQ.Heading>
@@ -180,96 +181,4 @@ describe('FAQ', () => {
     expect(subheadingEl).toBeInTheDocument()
     expect(questionheadingEl).toBeInTheDocument()
   })
-
-  it('renders the heading as a h3 and the question as a h4 by default', () => {
-    const {getByRole} = render(
-      <FAQ>
-        <FAQ.Heading>{mockHeading}</FAQ.Heading>
-        <FAQ.Item>
-          <FAQ.Question>{mockQuestion}</FAQ.Question>
-          <FAQ.Answer>
-            <p>{mockFAQAnswer}</p>
-          </FAQ.Answer>
-        </FAQ.Item>
-      </FAQ>,
-    )
-
-    const mainheadingEl = getByRole('heading', {level: 3, name: mockHeading})
-    const questionheadingEl = getByRole('heading', {level: 4, name: mockQuestion})
-
-    expect(mainheadingEl).toBeInTheDocument()
-    expect(questionheadingEl).toBeInTheDocument()
-  })
-
-  it('renders the heading as a h3, the subheading as a h4, and the question as a h5 when there is a subheading present', () => {
-    const {getByRole} = render(
-      <FAQ>
-        <FAQ.Heading>{mockHeading}</FAQ.Heading>
-        <FAQ.Subheading>{mockSubheading}</FAQ.Subheading>
-        <FAQ.Item>
-          <FAQ.Question>{mockQuestion}</FAQ.Question>
-          <FAQ.Answer>
-            <p>{mockFAQAnswer}</p>
-          </FAQ.Answer>
-        </FAQ.Item>
-      </FAQ>,
-    )
-
-    const mainheadingEl = getByRole('heading', {level: 3, name: mockHeading})
-    const subheadingEl = getByRole('heading', {level: 4, name: mockSubheading})
-    const questionheadingEl = getByRole('heading', {level: 5, name: mockQuestion})
-
-    expect(mainheadingEl).toBeInTheDocument()
-    expect(subheadingEl).toBeInTheDocument()
-    expect(questionheadingEl).toBeInTheDocument()
-  })
-
-  it('renders the heading as a h3, the subheading as a h4, and the question as a h5 when there is a subheading present, and the FAQ.Item is wrapped in a Fragment', () => {
-    const {getByRole} = render(
-      <FAQ>
-        <FAQ.Heading>{mockHeading}</FAQ.Heading>
-        <FAQ.Subheading>{mockSubheading}</FAQ.Subheading>
-        <>
-          <FAQ.Item key="item-1">
-            <FAQ.Question>{mockQuestion}</FAQ.Question>
-            <FAQ.Answer>
-              <p>{mockFAQAnswer}</p>
-            </FAQ.Answer>
-          </FAQ.Item>
-        </>
-      </FAQ>,
-    )
-
-    const mainheadingEl = getByRole('heading', {level: 3, name: mockHeading})
-    const subheadingEl = getByRole('heading', {level: 4, name: mockSubheading})
-    const questionheadingEl = getByRole('heading', {level: 5, name: mockQuestion})
-
-    expect(mainheadingEl).toBeInTheDocument()
-    expect(subheadingEl).toBeInTheDocument()
-    expect(questionheadingEl).toBeInTheDocument()
-  })
-
-  it('renders the heading as a h3, the subheading as a h4, and the question as a h5 when there is a subheading present, FAQ.Items are wrapped in a Fragment, and all FAQ.Items are wrapped in an array', () => {
-    const {getByRole} = render(
-      <FAQ>
-        <FAQ.Heading>{mockHeading}</FAQ.Heading>
-        <FAQ.Subheading>{mockSubheading}</FAQ.Subheading>
-        <>
-          {[1, 2].map(item => {
-            return (
-              <FAQ.Item key={`item-${item}`}>
-                <FAQ.Question>Question {item}</FAQ.Question>
-                <FAQ.Answer>Answer {item}</FAQ.Answer>
-              </FAQ.Item>
-            )
-          })}
-        </>
-      </FAQ>,
-    )
-
-    expect(getByRole('heading', {level: 3, name: mockHeading})).toBeInTheDocument()
-    expect(getByRole('heading', {level: 4, name: mockSubheading})).toBeInTheDocument()
-    expect(getByRole('heading', {level: 5, name: 'Question 1'})).toBeInTheDocument()
-    expect(getByRole('heading', {level: 5, name: 'Question 2'})).toBeInTheDocument()
-  })
 })

packages/react/src/FAQ/FAQ.tsx

@@ -22,30 +22,19 @@ export type FAQRootProps = PropsWithChildren<BaseProps<HTMLElement>> & React.HTM
 const FAQRoot = forwardRef<HTMLElement, FAQRootProps>(({children, style, animate, className, ...rest}, ref) => {
   const {classes: animationClasses, styles: animationInlineStyles} = useAnimation(animate)
 
-  const filteredChildren = React.Children.toArray(children)
-    .filter(child => {
-      if (React.isValidElement(child) && typeof child.type !== 'string') {
-        if (
-          isFragment(child) ||
-          (child as React.ReactElement).type === FAQHeading ||
-          (child as React.ReactElement).type === FAQSubheading ||
-          (child as React.ReactElement).type === AccordionRoot
-        ) {
-          return true
-        }
+  const filteredChildren = React.Children.toArray(children).filter(child => {
+    if (React.isValidElement(child) && typeof child.type !== 'string') {
+      if (
+        isFragment(child) ||
+        (child as React.ReactElement).type === FAQHeading ||
+        (child as React.ReactElement).type === FAQSubheading ||
+        (child as React.ReactElement).type === AccordionRoot
+      ) {
+        return true
       }
-      return false
-    })
-    .map(childMaybeFragment => {
-      /**
-       * If children are wrapped in a Fragment, extract the children.
-       * Children may also be an array of FAQ.Item components, so we call
-       * `toArray` to ensure we always have an array.
-       */
-      return React.Children.toArray(
-        isFragment(childMaybeFragment) ? childMaybeFragment.props.children : childMaybeFragment,
-      )
-    })
+    }
+    return false
+  })
 
   const hasSubheading = React.Children.toArray(children).some(
     child => React.isValidElement(child) && typeof child.type !== 'string' && child.type === FAQSubheading,
@@ -58,43 +47,17 @@ const FAQRoot = forwardRef<HTMLElement, FAQRootProps>(({children, style, animate
       style={{...animationInlineStyles, ...style}}
       {...rest}
     >
-      {filteredChildren.map(childArr => {
-        return childArr.map(child => {
-          if (!React.isValidElement(child) || typeof child.type === 'string') {
-            return child
-          }
-
+      {React.Children.toArray(filteredChildren).map(child => {
+        if (React.isValidElement(child) && typeof child.type !== 'string') {
           if (child.type === FAQHeading) {
             return React.cloneElement(child as React.ReactElement, {
               align: hasSubheading ? 'start' : child.props.align,
               size: hasSubheading ? '3' : child.props.size,
               weight: hasSubheading ? 'semibold' : child.props.weight,
             })
           }
-
-          // If there is a subheading, ensure that the FAQ.Question is rendered as a h5
-          if (hasSubheading && child.type === FAQ.Item) {
-            const grandChildren = React.Children.map(child.props.children, grandChild => {
-              if (!React.isValidElement(grandChild) || typeof grandChild.type === 'string') {
-                return grandChild
-              }
-
-              if (grandChild.type === FAQ.Question) {
-                return React.cloneElement(grandChild as React.ReactElement, {
-                  as: 'h5',
-                  ...(grandChild as React.ReactElement).props,
-                })
-              }
-              return grandChild
-            })
-
-            return React.cloneElement(child as React.ReactElement, {
-              children: grandChildren,
-            })
-          }
-
-          return child
-        })
+        }
+        return child
       })}
     </section>
   )
@@ -132,7 +95,7 @@ export type FAQSubheadingProps = BaseProps<HTMLHeadingElement> & {
 function FAQSubheading({
   children,
   className,
-  as = 'h4',
+  as = 'h3',
   size = 'subhead-large',
   weight = 'medium',
   ...rest

packages/react/src/FAQ/FAQGroup.test.tsx

@@ -164,48 +164,36 @@ describe('FAQGroup', () => {
     expect(getByRole('tab', {name: 'mock heading 3'})).toHaveAttribute('data-tab-heading', 'mock heading 3')
   })
 
-  it('renders the FAQGroup.Heading as a h3, the FAQ.Heading as a h4, and the question as a h5', () => {
-    const {getByRole, getAllByRole} = render(<Component />)
-
-    const faqGroupHeading = getByRole('heading', {level: 3, name: 'Frequently asked questions'})
-    const questionHeading = getByRole('heading', {level: 5, name: 'mock question 1'})
+  it('allows heading levels to be customized', () => {
+    const {getByRole, getAllByRole} = render(
+      <FAQGroup>
+        <FAQGroup.Heading as="h4">Frequently asked questions</FAQGroup.Heading>
+        <FAQ>
+          <FAQ.Heading as="h5">Mock heading</FAQ.Heading>
+          <FAQ.Item>
+            <FAQ.Question as="h6">Mock question</FAQ.Question>
+            <FAQ.Answer>
+              <p>Mock answer</p>
+            </FAQ.Answer>
+          </FAQ.Item>
+        </FAQ>
+      </FAQGroup>,
+    )
 
-    expect(faqGroupHeading).toBeInTheDocument()
-    expect(questionHeading).toBeInTheDocument()
+    expect(getByRole('heading', {level: 4, name: 'Frequently asked questions'})).toBeInTheDocument()
 
     /**
-     * We use getAllByRole here as we render two h4 elements with the same text content and hide one with media queries.
+     * We use getAllByRole here as we sometimes render two elements with the same text content and hide one with media queries.
      * Since our test suite doesn't render styles, Testing Library finds both of them, so we check both even though one is hidden.
      */
-    const faqHeadings = getAllByRole('heading', {level: 4, name: 'mock heading 1'})
+    const faqHeadings = getAllByRole('heading', {level: 5, name: 'Mock heading'})
     for (const faqHeading of faqHeadings) {
       expect(faqHeading).toBeInTheDocument()
     }
-  })
 
-  it('does not throw an error if an FAQGroup contains an FAQ which contains a fragment wrapping a falsey value', () => {
-    const {getByTestId} = render(
-      <FAQGroup data-testid="root">
-        <FAQGroup.Heading>Frequently asked questions</FAQGroup.Heading>
-        {testData.map((group, index) => (
-          <FAQ key={index}>
-            <FAQ.Heading>{group.heading}</FAQ.Heading>
-            <>
-              {false}
-              {group.faqs.map((faq, childIndex) => (
-                <FAQ.Item key={childIndex}>
-                  <FAQ.Question>{faq.question}</FAQ.Question>
-                  <FAQ.Answer>
-                    <p>{faq.answer}</p>
-                  </FAQ.Answer>
-                </FAQ.Item>
-              ))}
-            </>
-          </FAQ>
-        ))}
-      </FAQGroup>,
-    )
-
-    expect(getByTestId('root')).toBeInTheDocument()
+    const faqQuestions = getAllByRole('heading', {level: 6, name: 'Mock question'})
+    for (const faqQuestion of faqQuestions) {
+      expect(faqQuestion).toBeInTheDocument()
+    }
   })
 })