Fix invalid warning when using multiple Popover.Button components i…

…nside a `Popover.Panel` (#2333) * add a bunch of tests to ensure we won't regress on this again * fix incorrect warning when using multiple `Popover.Button` inside `Popover.Panel` * update changelog
tailwindlabs · Mar 3, 2023 · c759016 · c759016
1 parent 989cd6b
commit c759016
Show file tree

Hide file tree

Showing 3 changed files with 171 additions and 20 deletions.
diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Enable native label behavior for `<Switch>` where possible ([#2265](https://github.com/tailwindlabs/headlessui/pull/2265))
 - Allow root containers from the `Dialog` component in the `FocusTrap` component ([#2322](https://github.com/tailwindlabs/headlessui/pull/2322))
 - Fix `XYZPropsWeControl` and cleanup internal TypeScript types ([#2329](https://github.com/tailwindlabs/headlessui/pull/2329))
+- Fix invalid warning when using multiple `Popover.Button` components inside a `Popover.Panel` ([#2333](https://github.com/tailwindlabs/headlessui/pull/2333))
 
 ## [1.7.12] - 2023-02-24
 

diff --git a/packages/@headlessui-react/src/components/popover/popover.test.tsx b/packages/@headlessui-react/src/components/popover/popover.test.tsx
@@ -1,5 +1,5 @@
 import React, { createElement, useEffect, useRef, Fragment, useState } from 'react'
-import { render } from '@testing-library/react'
+import { render, act as _act } from '@testing-library/react'
 
 import { Popover } from './popover'
 import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs'
@@ -19,6 +19,8 @@ import { Portal } from '../portal/portal'
 import { Transition } from '../transitions/transition'
 import ReactDOM from 'react-dom'
 
+let act = _act as unknown as <T>(fn: () => T) => PromiseLike<T>
+
 jest.mock('../../hooks/use-id')
 
 afterAll(() => jest.restoreAllMocks())
@@ -777,6 +779,147 @@ describe('Rendering', () => {
       })
     )
   })
+
+  describe('Multiple `Popover.Button` warnings', () => {
+    let spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
+    beforeEach(() => {
+      spy.mockRestore()
+      spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
+    })
+    afterEach(() => {
+      spy.mockRestore()
+    })
+
+    it('should warn when you are using multiple `Popover.Button` components', async () => {
+      render(
+        <Popover>
+          <Popover.Button>Button #1</Popover.Button>
+          <Popover.Button>Button #2</Popover.Button>
+          <Popover.Panel>Popover panel</Popover.Panel>
+        </Popover>
+      )
+
+      // Open Popover
+      await click(getPopoverButton())
+
+      expect(spy).toHaveBeenCalledWith(
+        'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
+      )
+    })
+
+    it('should warn when you are using multiple `Popover.Button` components (wrapped in a Transition)', async () => {
+      render(
+        <Popover>
+          <Popover.Button>Button #1</Popover.Button>
+          <Popover.Button>Button #2</Popover.Button>
+          <Transition>
+            <Popover.Panel>Popover panel</Popover.Panel>
+          </Transition>
+        </Popover>
+      )
+
+      // Open Popover
+      await act(() => click(getPopoverButton()))
+
+      expect(spy).toHaveBeenCalledWith(
+        'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
+      )
+    })
+
+    it('should not warn when you are using multiple `Popover.Button` components inside the `Popover.Panel`', async () => {
+      render(
+        <Popover>
+          <Popover.Button>Button #1</Popover.Button>
+          <Popover.Panel>
+            <Popover.Button>Close #1</Popover.Button>
+            <Popover.Button>Close #2</Popover.Button>
+          </Popover.Panel>
+        </Popover>
+      )
+
+      // Open Popover
+      await click(getPopoverButton())
+
+      expect(spy).not.toHaveBeenCalledWith(
+        'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
+      )
+    })
+
+    it('should not warn when you are using multiple `Popover.Button` components inside the `Popover.Panel` (wrapped in a Transition)', async () => {
+      render(
+        <Popover>
+          <Popover.Button>Button #1</Popover.Button>
+          <Transition>
+            <Popover.Panel>
+              <Popover.Button>Close #1</Popover.Button>
+              <Popover.Button>Close #2</Popover.Button>
+            </Popover.Panel>
+          </Transition>
+        </Popover>
+      )
+
+      // Open Popover
+      await act(() => click(getPopoverButton()))
+
+      expect(spy).not.toHaveBeenCalledWith(
+        'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
+      )
+    })
+
+    it('should warn when you are using multiple `Popover.Button` components in a nested `Popover`', async () => {
+      render(
+        <Popover>
+          <Popover.Button>Button #1</Popover.Button>
+          <Popover.Panel>
+            Popover panel #1
+            <Popover>
+              <Popover.Button>Button #2</Popover.Button>
+              <Popover.Button>Button #3</Popover.Button>
+              <Popover.Panel>Popover panel #2</Popover.Panel>
+            </Popover>
+          </Popover.Panel>
+        </Popover>
+      )
+
+      // Open the first Popover
+      await click(getByText('Button #1'))
+
+      // Open the second Popover
+      await click(getByText('Button #2'))
+
+      expect(spy).toHaveBeenCalledWith(
+        'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
+      )
+    })
+
+    it('should not warn when you are using multiple `Popover.Button` components in a nested `Popover.Panel`', async () => {
+      render(
+        <Popover>
+          <Popover.Button>Button #1</Popover.Button>
+          <Popover.Panel>
+            Popover panel #1
+            <Popover>
+              <Popover.Button>Button #2</Popover.Button>
+              <Popover.Panel>
+                <Popover.Button>Button #3</Popover.Button>
+                <Popover.Button>Button #4</Popover.Button>
+              </Popover.Panel>
+            </Popover>
+          </Popover.Panel>
+        </Popover>
+      )
+
+      // Open the first Popover
+      await click(getByText('Button #1'))
+
+      // Open the second Popover
+      await click(getByText('Button #2'))
+
+      expect(spy).not.toHaveBeenCalledWith(
+        'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
+      )
+    })
+  })
 })
 
 describe('Composition', () => {

diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx
@@ -358,24 +358,26 @@ function PopoverFn<TTag extends ElementType = typeof DEFAULT_POPOVER_TAG>(
   let ourProps = { ref: popoverRef }
 
   return (
-    <PopoverContext.Provider value={reducerBag}>
-      <PopoverAPIContext.Provider value={api}>
-        <OpenClosedProvider
-          value={match(popoverState, {
-            [PopoverStates.Open]: State.Open,
-            [PopoverStates.Closed]: State.Closed,
-          })}
-        >
-          {render({
-            ourProps,
-            theirProps,
-            slot,
-            defaultTag: DEFAULT_POPOVER_TAG,
-            name: 'Popover',
-          })}
-        </OpenClosedProvider>
-      </PopoverAPIContext.Provider>
-    </PopoverContext.Provider>
+    <PopoverPanelContext.Provider value={null}>
+      <PopoverContext.Provider value={reducerBag}>
+        <PopoverAPIContext.Provider value={api}>
+          <OpenClosedProvider
+            value={match(popoverState, {
+              [PopoverStates.Open]: State.Open,
+              [PopoverStates.Closed]: State.Closed,
+            })}
+          >
+            {render({
+              ourProps,
+              theirProps,
+              slot,
+              defaultTag: DEFAULT_POPOVER_TAG,
+              name: 'Popover',
+            })}
+          </OpenClosedProvider>
+        </PopoverAPIContext.Provider>
+      </PopoverContext.Provider>
+    </PopoverPanelContext.Provider>
   )
 }
 
@@ -417,7 +419,12 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
   // if a `Popover.Button` is rendered inside a `Popover` which in turn is rendered inside a
   // `Popover.Panel` (aka nested popovers), then we need to make sure that the button is able to
   // open the nested popover.
-  let isWithinPanel = panelContext === null ? false : panelContext === state.panelId
+  //
+  // The `Popover` itself will also render a `PopoverPanelContext` but with a value of `null`. That
+  // way we don't need to keep track of _which_ `Popover.Panel` (if at all) we are in, we can just
+  // check if we are in a `Popover.Panel` or not since this will always point to the nearest one and
+  // won't pierce through `Popover` components themselves.
+  let isWithinPanel = panelContext !== null
 
   useEffect(() => {
     if (isWithinPanel) return