Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid warning when using multiple Popover.Button components inside a Popover.Panel #2333

Merged
merged 3 commits into from
Mar 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@headlessui-react/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
145 changes: 144 additions & 1 deletion packages/@headlessui-react/src/components/popover/popover.test.tsx
Original file line number Diff line number Diff line change
@@ -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'
Expand All @@ -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())
Expand Down Expand Up @@ -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', () => {
Expand Down
45 changes: 26 additions & 19 deletions packages/@headlessui-react/src/components/popover/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
)
}

Expand Down Expand Up @@ -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
Expand Down