From c759016fa351892b192e30623c33b8610b403bc4 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Fri, 3 Mar 2023 14:59:10 +0100 Subject: [PATCH] Fix invalid warning when using multiple `Popover.Button` components inside 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 --- packages/@headlessui-react/CHANGELOG.md | 1 + .../src/components/popover/popover.test.tsx | 145 +++++++++++++++++- .../src/components/popover/popover.tsx | 45 +++--- 3 files changed, 171 insertions(+), 20 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 9d3e9edff2..000d2c0b29 100644 --- 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 `` 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 index 444f7abe5e..c43414993a 100644 --- 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 (fn: () => T) => PromiseLike + 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( + + Button #1 + Button #2 + Popover panel + + ) + + // Open Popover + await click(getPopoverButton()) + + expect(spy).toHaveBeenCalledWith( + 'You are already using a but only 1 is supported.' + ) + }) + + it('should warn when you are using multiple `Popover.Button` components (wrapped in a Transition)', async () => { + render( + + Button #1 + Button #2 + + Popover panel + + + ) + + // Open Popover + await act(() => click(getPopoverButton())) + + expect(spy).toHaveBeenCalledWith( + 'You are already using a but only 1 is supported.' + ) + }) + + it('should not warn when you are using multiple `Popover.Button` components inside the `Popover.Panel`', async () => { + render( + + Button #1 + + Close #1 + Close #2 + + + ) + + // Open Popover + await click(getPopoverButton()) + + expect(spy).not.toHaveBeenCalledWith( + 'You are already using a but only 1 is supported.' + ) + }) + + it('should not warn when you are using multiple `Popover.Button` components inside the `Popover.Panel` (wrapped in a Transition)', async () => { + render( + + Button #1 + + + Close #1 + Close #2 + + + + ) + + // Open Popover + await act(() => click(getPopoverButton())) + + expect(spy).not.toHaveBeenCalledWith( + 'You are already using a but only 1 is supported.' + ) + }) + + it('should warn when you are using multiple `Popover.Button` components in a nested `Popover`', async () => { + render( + + Button #1 + + Popover panel #1 + + Button #2 + Button #3 + Popover panel #2 + + + + ) + + // 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 but only 1 is supported.' + ) + }) + + it('should not warn when you are using multiple `Popover.Button` components in a nested `Popover.Panel`', async () => { + render( + + Button #1 + + Popover panel #1 + + Button #2 + + Button #3 + Button #4 + + + + + ) + + // 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 but only 1 is supported.' + ) + }) + }) }) describe('Composition', () => { diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx index 61b2cf083e..f626789e2f 100644 --- a/packages/@headlessui-react/src/components/popover/popover.tsx +++ b/packages/@headlessui-react/src/components/popover/popover.tsx @@ -358,24 +358,26 @@ function PopoverFn( let ourProps = { ref: popoverRef } return ( - - - - {render({ - ourProps, - theirProps, - slot, - defaultTag: DEFAULT_POPOVER_TAG, - name: 'Popover', - })} - - - + + + + + {render({ + ourProps, + theirProps, + slot, + defaultTag: DEFAULT_POPOVER_TAG, + name: 'Popover', + })} + + + + ) } @@ -417,7 +419,12 @@ function ButtonFn( // 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