Skip to content

Commit

Permalink
Fix incorrect Transition boundary for Dialog component (#3331)
Browse files Browse the repository at this point in the history
* always wrap `Dialog` in a `Transition`

Initially we didn't do this, because it's a bit silly to do that if you
already had a `Transition` component on the outside. E.g.:

```tsx
<Transition show={open}>
  <Dialog onClose={() => setOpen(false)}>
    {/* ... */}
  </Dialog>
</Transition>
```

Because this means that we technically have this:
```tsx
<Transition show={open}>
  <Dialog onClose={() => setOpen(false)}>
    <Transition>
      <InternalDialog>
        {/* ... */}
      </InternalDialog>
    </Dialog>
  </Transition>
</Transition>
```

The good part is that the inner `Transition` is rendering a `Fragment`
and forwards all the props to the underlying element (the internal
dialog).

This way we have a guaranteed transition boundary.

* use public `transition` API instead of private internal API

This also mimics better what we are actually trying to do.

* update changelog
  • Loading branch information
RobinMalfait committed Jun 26, 2024
1 parent 275c205 commit 3224a9e
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 14 deletions.
1 change: 1 addition & 0 deletions packages/@headlessui-react/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fix issues spreading omitted props onto components ([#3313](https://github.com/tailwindlabs/headlessui/pull/3313))
- Fix initial `anchor="selection"` positioning ([#3324](https://github.com/tailwindlabs/headlessui/pull/3324))
- Fix render prop in `ComboboxOptions` to use `any` instead of `unknown` ([#3327](https://github.com/tailwindlabs/headlessui/pull/3327))
- Fix incorrect `Transition` boundary for `Dialog` component ([#3331](https://github.com/tailwindlabs/headlessui/pull/3331))

## [2.1.0] - 2024-06-21

Expand Down
49 changes: 37 additions & 12 deletions packages/@headlessui-react/src/components/dialog/dialog.test.tsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,20 @@
import { render } from '@testing-library/react'
import React, { createElement, Fragment, useCallback, useEffect, useRef, useState } from 'react'
import React, { Fragment, createElement, useCallback, useEffect, useRef, useState } from 'react'
import { createPortal } from 'react-dom'
import { OpenClosedProvider, State } from '../../internal/open-closed'
import {
DialogState,
PopoverState,
assertActiveElement,
assertDialog,
assertDialogDescription,
assertDialogTitle,
assertPopoverPanel,
DialogState,
getByText,
getDialog,
getDialogs,
getPopoverButton,
PopoverState,
} from '../../test-utils/accessibility-assertions'
import { click, focus, Keys, mouseDrag, press, shift } from '../../test-utils/interactions'
import { Keys, click, focus, mouseDrag, press, shift } from '../../test-utils/interactions'
import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs'
import type { PropsOf } from '../../types'
import { Popover } from '../popover/popover'
Expand Down Expand Up @@ -498,25 +497,51 @@ describe('Rendering', () => {
it(
'should remove the scroll lock when the open closed state is `Closing`',
suppressConsoleLogs(async () => {
function Example({ value = State.Open }) {
function Example({ open = true }) {
return (
<Dialog transition autoFocus={false} open={open} onClose={() => {}}>
<input id="a" type="text" />
<input id="b" type="text" />
<input id="c" type="text" />
</Dialog>
)
}

let { rerender } = render(<Example open={true} />)

// The overflow should be there
expect(document.documentElement.style.overflow).toBe('hidden')

// Re-render but with an exit transition
rerender(<Example open={false} />)

// The moment the dialog is closing, the overflow should be gone
expect(document.documentElement.style.overflow).toBe('')
})
)

it(
'should remove the scroll lock when the open closed state is `Closing` (using Transition wrapper)',
suppressConsoleLogs(async () => {
function Example({ open = true }) {
return (
<OpenClosedProvider value={value}>
<Dialog autoFocus={false} open={true} onClose={() => {}}>
<Transition show={open}>
<Dialog autoFocus={false} onClose={() => {}}>
<input id="a" type="text" />
<input id="b" type="text" />
<input id="c" type="text" />
</Dialog>
</OpenClosedProvider>
</Transition>
)
}

let { rerender } = render(<Example value={State.Open} />)
let { rerender } = render(<Example open={true} />)

// The overflow should be there
expect(document.documentElement.style.overflow).toBe('hidden')

// Re-render but with the `Closing` state
rerender(<Example value={State.Open | State.Closing} />)
// Re-render but with an exit transition
rerender(<Example open={false} />)

// The moment the dialog is closing, the overflow should be gone
expect(document.documentElement.style.overflow).toBe('')
Expand Down
3 changes: 1 addition & 2 deletions packages/@headlessui-react/src/components/dialog/dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,7 @@ function DialogFn<TTag extends ElementType = typeof DEFAULT_DIALOG_TAG>(
)
}

let inTransitionComponent = usesOpenClosedState !== null
if (!inTransitionComponent && open !== undefined && !rest.static) {
if ((open !== undefined || transition) && !rest.static) {
return (
<Transition show={open} transition={transition} unmount={rest.unmount}>
<InternalDialog ref={ref} {...rest} />
Expand Down

0 comments on commit 3224a9e

Please sign in to comment.