Skip to content

Commit c759016

Browse files
RobinMalfaitRobinMalfait
authored
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
1 parent 989cd6b commit c759016

File tree

3 files changed

+171
-20
lines changed

3 files changed

+171
-20
lines changed

‎packages/@headlessui-react/CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1313
- Enable native label behavior for `<Switch>` where possible ([#2265](https://github.com/tailwindlabs/headlessui/pull/2265))
1414
- Allow root containers from the `Dialog` component in the `FocusTrap` component ([#2322](https://github.com/tailwindlabs/headlessui/pull/2322))
1515
- Fix `XYZPropsWeControl` and cleanup internal TypeScript types ([#2329](https://github.com/tailwindlabs/headlessui/pull/2329))
16+
- Fix invalid warning when using multiple `Popover.Button` components inside a `Popover.Panel` ([#2333](https://github.com/tailwindlabs/headlessui/pull/2333))
1617

1718
## [1.7.12] - 2023-02-24
1819

‎packages/@headlessui-react/src/components/popover/popover.test.tsx

+144-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import React, { createElement, useEffect, useRef, Fragment, useState } from 'react'
2-
import { render } from '@testing-library/react'
2+
import { render, act as _act } from '@testing-library/react'
33

44
import { Popover } from './popover'
55
import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs'
@@ -19,6 +19,8 @@ import { Portal } from '../portal/portal'
1919
import { Transition } from '../transitions/transition'
2020
import ReactDOM from 'react-dom'
2121

22+
let act = _act as unknown as <T>(fn: () => T) => PromiseLike<T>
23+
2224
jest.mock('../../hooks/use-id')
2325

2426
afterAll(() => jest.restoreAllMocks())
@@ -777,6 +779,147 @@ describe('Rendering', () => {
777779
})
778780
)
779781
})
782+
783+
describe('Multiple `Popover.Button` warnings', () => {
784+
let spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
785+
beforeEach(() => {
786+
spy.mockRestore()
787+
spy = jest.spyOn(console, 'warn').mockImplementation(() => {})
788+
})
789+
afterEach(() => {
790+
spy.mockRestore()
791+
})
792+
793+
it('should warn when you are using multiple `Popover.Button` components', async () => {
794+
render(
795+
<Popover>
796+
<Popover.Button>Button #1</Popover.Button>
797+
<Popover.Button>Button #2</Popover.Button>
798+
<Popover.Panel>Popover panel</Popover.Panel>
799+
</Popover>
800+
)
801+
802+
// Open Popover
803+
await click(getPopoverButton())
804+
805+
expect(spy).toHaveBeenCalledWith(
806+
'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
807+
)
808+
})
809+
810+
it('should warn when you are using multiple `Popover.Button` components (wrapped in a Transition)', async () => {
811+
render(
812+
<Popover>
813+
<Popover.Button>Button #1</Popover.Button>
814+
<Popover.Button>Button #2</Popover.Button>
815+
<Transition>
816+
<Popover.Panel>Popover panel</Popover.Panel>
817+
</Transition>
818+
</Popover>
819+
)
820+
821+
// Open Popover
822+
await act(() => click(getPopoverButton()))
823+
824+
expect(spy).toHaveBeenCalledWith(
825+
'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
826+
)
827+
})
828+
829+
it('should not warn when you are using multiple `Popover.Button` components inside the `Popover.Panel`', async () => {
830+
render(
831+
<Popover>
832+
<Popover.Button>Button #1</Popover.Button>
833+
<Popover.Panel>
834+
<Popover.Button>Close #1</Popover.Button>
835+
<Popover.Button>Close #2</Popover.Button>
836+
</Popover.Panel>
837+
</Popover>
838+
)
839+
840+
// Open Popover
841+
await click(getPopoverButton())
842+
843+
expect(spy).not.toHaveBeenCalledWith(
844+
'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
845+
)
846+
})
847+
848+
it('should not warn when you are using multiple `Popover.Button` components inside the `Popover.Panel` (wrapped in a Transition)', async () => {
849+
render(
850+
<Popover>
851+
<Popover.Button>Button #1</Popover.Button>
852+
<Transition>
853+
<Popover.Panel>
854+
<Popover.Button>Close #1</Popover.Button>
855+
<Popover.Button>Close #2</Popover.Button>
856+
</Popover.Panel>
857+
</Transition>
858+
</Popover>
859+
)
860+
861+
// Open Popover
862+
await act(() => click(getPopoverButton()))
863+
864+
expect(spy).not.toHaveBeenCalledWith(
865+
'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
866+
)
867+
})
868+
869+
it('should warn when you are using multiple `Popover.Button` components in a nested `Popover`', async () => {
870+
render(
871+
<Popover>
872+
<Popover.Button>Button #1</Popover.Button>
873+
<Popover.Panel>
874+
Popover panel #1
875+
<Popover>
876+
<Popover.Button>Button #2</Popover.Button>
877+
<Popover.Button>Button #3</Popover.Button>
878+
<Popover.Panel>Popover panel #2</Popover.Panel>
879+
</Popover>
880+
</Popover.Panel>
881+
</Popover>
882+
)
883+
884+
// Open the first Popover
885+
await click(getByText('Button #1'))
886+
887+
// Open the second Popover
888+
await click(getByText('Button #2'))
889+
890+
expect(spy).toHaveBeenCalledWith(
891+
'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
892+
)
893+
})
894+
895+
it('should not warn when you are using multiple `Popover.Button` components in a nested `Popover.Panel`', async () => {
896+
render(
897+
<Popover>
898+
<Popover.Button>Button #1</Popover.Button>
899+
<Popover.Panel>
900+
Popover panel #1
901+
<Popover>
902+
<Popover.Button>Button #2</Popover.Button>
903+
<Popover.Panel>
904+
<Popover.Button>Button #3</Popover.Button>
905+
<Popover.Button>Button #4</Popover.Button>
906+
</Popover.Panel>
907+
</Popover>
908+
</Popover.Panel>
909+
</Popover>
910+
)
911+
912+
// Open the first Popover
913+
await click(getByText('Button #1'))
914+
915+
// Open the second Popover
916+
await click(getByText('Button #2'))
917+
918+
expect(spy).not.toHaveBeenCalledWith(
919+
'You are already using a <Popover.Button /> but only 1 <Popover.Button /> is supported.'
920+
)
921+
})
922+
})
780923
})
781924

782925
describe('Composition', () => {

‎packages/@headlessui-react/src/components/popover/popover.tsx

+26-19
Original file line numberDiff line numberDiff line change
@@ -358,24 +358,26 @@ function PopoverFn<TTag extends ElementType = typeof DEFAULT_POPOVER_TAG>(
358358
let ourProps = { ref: popoverRef }
359359

360360
return (
361-
<PopoverContext.Provider value={reducerBag}>
362-
<PopoverAPIContext.Provider value={api}>
363-
<OpenClosedProvider
364-
value={match(popoverState, {
365-
[PopoverStates.Open]: State.Open,
366-
[PopoverStates.Closed]: State.Closed,
367-
})}
368-
>
369-
{render({
370-
ourProps,
371-
theirProps,
372-
slot,
373-
defaultTag: DEFAULT_POPOVER_TAG,
374-
name: 'Popover',
375-
})}
376-
</OpenClosedProvider>
377-
</PopoverAPIContext.Provider>
378-
</PopoverContext.Provider>
361+
<PopoverPanelContext.Provider value={null}>
362+
<PopoverContext.Provider value={reducerBag}>
363+
<PopoverAPIContext.Provider value={api}>
364+
<OpenClosedProvider
365+
value={match(popoverState, {
366+
[PopoverStates.Open]: State.Open,
367+
[PopoverStates.Closed]: State.Closed,
368+
})}
369+
>
370+
{render({
371+
ourProps,
372+
theirProps,
373+
slot,
374+
defaultTag: DEFAULT_POPOVER_TAG,
375+
name: 'Popover',
376+
})}
377+
</OpenClosedProvider>
378+
</PopoverAPIContext.Provider>
379+
</PopoverContext.Provider>
380+
</PopoverPanelContext.Provider>
379381
)
380382
}
381383

@@ -417,7 +419,12 @@ function ButtonFn<TTag extends ElementType = typeof DEFAULT_BUTTON_TAG>(
417419
// if a `Popover.Button` is rendered inside a `Popover` which in turn is rendered inside a
418420
// `Popover.Panel` (aka nested popovers), then we need to make sure that the button is able to
419421
// open the nested popover.
420-
let isWithinPanel = panelContext === null ? false : panelContext === state.panelId
422+
//
423+
// The `Popover` itself will also render a `PopoverPanelContext` but with a value of `null`. That
424+
// way we don't need to keep track of _which_ `Popover.Panel` (if at all) we are in, we can just
425+
// check if we are in a `Popover.Panel` or not since this will always point to the nearest one and
426+
// won't pierce through `Popover` components themselves.
427+
let isWithinPanel = panelContext !== null
421428

422429
useEffect(() => {
423430
if (isWithinPanel) return

0 commit comments

Comments
 (0)