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

Lazily resolve default containers in <Dialog> #2697

Merged
merged 2 commits into from
Aug 22, 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.
Jump to
Jump to file
Failed to load files.
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 @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Don't call `<Dialog>`'s `onClose` twice on mobile devices ([#2690](https://github.com/tailwindlabs/headlessui/pull/2690))
- Lazily resolve default containers in `<Dialog>` ([#2697](https://github.com/tailwindlabs/headlessui/pull/2697))

## [1.7.17] - 2023-08-17

Expand Down
37 changes: 37 additions & 0 deletions packages/@headlessui-react/src/components/dialog/dialog.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1525,6 +1525,43 @@ describe('Mouse interactions', () => {
})
)

// NOTE: This test doesn't actually fail in JSDOM when it's supposed to
// We're keeping it around for documentation purposes
it(
'should not close the Dialog if it starts open and we click inside the Dialog when it has only a panel',
suppressConsoleLogs(async () => {
function Example() {
let [isOpen, setIsOpen] = useState(true)
return (
<>
<button id="trigger" onClick={() => setIsOpen((v) => !v)}>
Trigger
</button>
<Dialog open={isOpen} onClose={() => setIsOpen(false)}>
<Dialog.Panel>
<p id="inside">My content</p>
<button>close</button>
</Dialog.Panel>
</Dialog>
</>
)
}

render(<Example />)

// Open the dialog
await click(document.getElementById('trigger'))

assertDialog({ state: DialogState.Visible })

// Click the p tag inside the dialog
await click(document.getElementById('inside'))

// It should not have closed
assertDialog({ state: DialogState.Visible })
})
)

it(
'should close the Dialog if we click outside the Dialog.Panel',
suppressConsoleLogs(async () => {
Expand Down
14 changes: 13 additions & 1 deletion packages/@headlessui-react/src/components/dialog/dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import React, {
ContextType,
ElementType,
MouseEvent as ReactMouseEvent,
RefObject,
MutableRefObject,
Ref,
} from 'react'
Expand Down Expand Up @@ -210,13 +211,24 @@ function DialogFn<TTag extends ElementType = typeof DEFAULT_DIALOG_TAG>(
let hasNestedDialogs = nestedDialogCount > 1 // 1 is the current dialog
let hasParentDialog = useContext(DialogContext) !== null
let [portals, PortalWrapper] = useNestedPortals()

// We use this because reading these values during iniital render(s)
// can result in `null` rather then the actual elements
// This doesn't happen when using certain components like a
// `<Dialog.Title>` because they cause the parent to re-render
let defaultContainer: RefObject<HTMLElement> = {
get current() {
return state.panelRef.current ?? internalDialogRef.current
},
}

let {
resolveContainers: resolveRootContainers,
mainTreeNodeRef,
MainTreeNode,
} = useRootContainers({
portals,
defaultContainers: [state.panelRef.current ?? internalDialogRef.current],
defaultContainers: [defaultContainer],
})

// If there are multiple dialogs, then you can be the root, the leaf or one
Expand Down