huanld/headlessui
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Change default tag from div to Fragment on Transition components (#3110)

Browse Source 
* use `Fragment` as the default element for `Transition` components

* update tests to reflect default tag change

* only error on missing `ref` if it's actually required

If the `<Transition />` component "root" is used as a root placeholder
(for state management) and not making actual transitions itself, then we
don't require a `ref` element.

* add test to ensure we don't error on missing `ref` when not required

+ add `className="…"` to some places to indicate that we _do_ want to
  perform a transition and thus have to fail if the `ref` is missing.

* improve `requiresRef` check

Also ensure that a ref is required if the `as` prop is provided and
it's not a `Fragment`

* add `shouldForwardRef` helper

* fix broken tests

These tests were rendering a `Debug` element that didn't render any DOM
nodes. Adding `as="div"` ensures that we are forwarding the ref
correctly.

* update changelog

* update playgrounds to reflect tag change

* Tweak changelog

---------

Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
This commit is contained in:
Robin Malfait
2024-04-19 16:15:11 +02:00
committed by GitHub
parent 83cda0aa75
commit 8fa5caf0dc
22 changed files with 182 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx
+1 -1
View File
@@ -597,7 +597,7 @@ describe('Composition', () => {
<Transition>
<Debug name="Transition" fn={orderFn} />
<Disclosure.Panel>
<Transition.Child>
<Transition.Child as="div">
<Debug name="Transition.Child" fn={orderFn} />
</Transition.Child>
</Disclosure.Panel>
packages/@headlessui-react/src/components/popover/popover.test.tsx
+1 -1
View File
@@ -977,7 +977,7 @@ describe('Composition', () => {
<Transition>
<Debug name="Transition" fn={orderFn} />
<Popover.Panel>
<Transition.Child>
<Transition.Child as="div">
<Debug name="Transition.Child" fn={orderFn} />
</Transition.Child>
</Popover.Panel>
packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap
-6
View File
@@ -111,12 +111,6 @@ exports[`Setup API shallow should passthrough all the props (that we do not use
</a>
`;
exports[`Setup API shallow should render a div and its children by default 1`] = `
<div>
Children
</div>
`;
exports[`Setup API shallow should render another component if the \`as\` prop is used and its children by default 1`] = `
<a>
Children
packages/@headlessui-react/src/components/transition/transition.test.tsx
+99 -51
View File
@@ -1,5 +1,5 @@
import { act as _act, fireEvent, render } from '@testing-library/react'
import React, { Fragment, useEffect, useLayoutEffect, useRef, useState } from 'react'
import React, { useEffect, useLayoutEffect, useRef, useState } from 'react'
import { getByText } from '../../test-utils/accessibility-assertions'
import { executeTimeline } from '../../test-utils/execute-timeline'
import { click } from '../../test-utils/interactions'
@@ -22,7 +22,7 @@ function nextFrame() {
it('should not steal the ref from the child', async () => {
let fn = jest.fn()
render(
<Transition show={true} as={Fragment}>
<Transition show={true}>
<div ref={fn}>...</div>
</Transition>
)
@@ -40,11 +40,6 @@ it('should render without crashing', () => {
)
})
it('should be possible to render a Transition without children', () => {
render(<Transition show={true} className="transition" />)
expect(document.getElementsByClassName('transition')).not.toBeNull()
})
it(
'should yell at us when we forget the required show prop',
suppressConsoleLogs(() => {
@@ -62,16 +57,15 @@ it(
describe('Setup API', () => {
describe('shallow', () => {
it('should render a div and its children by default', () => {
let { container } = render(<Transition show={true}>Children</Transition>)
expect(container.firstChild).toMatchSnapshot()
})
it('should passthrough all the props (that we do not use internally)', () => {
let { container } = render(
/**
* Renders a Fragment by default and forwards props. But not possible to
* type in TypeScript land. This is also discouraged, but it works.
*/
// @ts-expect-error
<Transition show={true} id="root" className="text-blue-400">
Children
<div>Children</div>
</Transition>
)
@@ -99,7 +93,11 @@ describe('Setup API', () => {
})
it('should render nothing when the show prop is false', () => {
let { container } = render(<Transition show={false}>Children</Transition>)
let { container } = render(
<Transition show={false}>
<div>Children</div>
</Transition>
)
expect(container.firstChild).toMatchSnapshot()
})
@@ -115,11 +113,7 @@ describe('Setup API', () => {
})
it('should be possible to use a render prop', () => {
let { container } = render(
<Transition show={true} as={Fragment}>
{() => <span>Children</span>}
</Transition>
)
let { container } = render(<Transition show={true}>{() => <span>Children</span>}</Transition>)
expect(container.firstChild).toMatchSnapshot()
})
@@ -135,7 +129,7 @@ describe('Setup API', () => {
expect(() => {
render(
<Transition show={true} as={Fragment}>
<Transition show={true} enter="duration-200">
{() => <Dummy />}
</Transition>
)
@@ -153,7 +147,9 @@ describe('Setup API', () => {
expect(() => {
render(
<div className="My Page">
<Transition.Child>Oops</Transition.Child>
<Transition.Child>
<div>Oops</div>
</Transition.Child>
</div>
)
}).toThrowErrorMatchingSnapshot()
@@ -163,7 +159,9 @@ describe('Setup API', () => {
it('should be possible to render a Transition.Child without children', () => {
render(
<Transition show={true}>
<Transition.Child className="transition" />
<Transition.Child>
<div className="transition" />
</Transition.Child>
</Transition>
)
expect(document.getElementsByClassName('transition')).not.toBeNull()
@@ -172,7 +170,9 @@ describe('Setup API', () => {
it('should be possible to use a Transition.Root and a Transition.Child', () => {
render(
<Transition.Root show={true}>
<Transition.Child className="transition" />
<Transition.Child>
<div className="transition" />
</Transition.Child>
</Transition.Root>
)
expect(document.getElementsByClassName('transition')).not.toBeNull()
@@ -182,8 +182,14 @@ describe('Setup API', () => {
let { container } = render(
<div className="My Page">
<Transition show={true}>
<Transition.Child>Sidebar</Transition.Child>
<Transition.Child>Content</Transition.Child>
<div>
<Transition.Child>
<div>Sidebar</div>
</Transition.Child>
<Transition.Child>
<div>Content</div>
</Transition.Child>
</div>
</Transition>
</div>
)
@@ -195,8 +201,10 @@ describe('Setup API', () => {
let { container } = render(
<div className="My Page">
<Transition show={true}>
<Transition.Child as="aside">Sidebar</Transition.Child>
<Transition.Child as="section">Content</Transition.Child>
<div>
<Transition.Child as="aside">Sidebar</Transition.Child>
<Transition.Child as="section">Content</Transition.Child>
</div>
</Transition>
</div>
)
@@ -221,8 +229,10 @@ describe('Setup API', () => {
let { container } = render(
<div className="My Page">
<Transition show={true}>
<Transition.Child as={Fragment}>{() => <aside>Sidebar</aside>}</Transition.Child>
<Transition.Child as={Fragment}>{() => <section>Content</section>}</Transition.Child>
<div>
<Transition.Child>{() => <aside>Sidebar</aside>}</Transition.Child>
<Transition.Child>{() => <section>Content</section>}</Transition.Child>
</div>
</Transition>
</div>
)
@@ -233,13 +243,11 @@ describe('Setup API', () => {
it('should be possible to use render props on the Transition and Transition.Child components', () => {
let { container } = render(
<div className="My Page">
<Transition show={true} as={Fragment}>
<Transition show={true}>
{() => (
<article>
<Transition.Child as={Fragment}>{() => <aside>Sidebar</aside>}</Transition.Child>
<Transition.Child as={Fragment}>
{() => <section>Content</section>}
</Transition.Child>
<Transition.Child>{() => <aside>Sidebar</aside>}</Transition.Child>
<Transition.Child>{() => <section>Content</section>}</Transition.Child>
</article>
)}
</Transition>
@@ -262,8 +270,14 @@ describe('Setup API', () => {
render(
<div className="My Page">
<Transition show={true}>
<Transition.Child as={Fragment}>{() => <Dummy>Sidebar</Dummy>}</Transition.Child>
<Transition.Child as={Fragment}>{() => <Dummy>Content</Dummy>}</Transition.Child>
<div>
<Transition.Child enter="duration-200">
{() => <Dummy>Sidebar</Dummy>}
</Transition.Child>
<Transition.Child enter="duration-200">
{() => <Dummy>Content</Dummy>}
</Transition.Child>
</div>
</Transition>
</div>
)
@@ -271,6 +285,28 @@ describe('Setup API', () => {
})
)
it(
'should not error when the `Transition` component does not contain any props that need to be forwarded',
suppressConsoleLogs(() => {
expect.assertions(1)
expect(() => {
render(
<div className="My Page">
<Transition show={true}>
<Transition.Child>
<div>Sidebar</div>
</Transition.Child>
<Transition.Child>
<div>Content</div>
</Transition.Child>
</Transition>
</div>
)
}).not.toThrow()
})
)
it(
'should yell at us when we forgot to forward a ref on the Transition component',
suppressConsoleLogs(() => {
@@ -283,7 +319,7 @@ describe('Setup API', () => {
expect(() => {
render(
<div className="My Page">
<Transition show={true} as={Fragment}>
<Transition show={true} enter="duration-200">
{() => (
<Dummy>
<Transition.Child>{() => <aside>Sidebar</aside>}</Transition.Child>
@@ -362,7 +398,7 @@ describe('Setup API', () => {
leaveFrom="leave-from"
leaveTo="leave-to"
>
Children
<div>Children</div>
</Transition>
)
@@ -387,7 +423,7 @@ describe('Setup API', () => {
leaveFrom="leave-from"
leaveTo="leave-to"
>
Children
<div>Children</div>
</Transition>
)
}
@@ -732,10 +768,10 @@ describe('Transitions', () => {
<Transition show={show}>
<Transition.Child leave="leave-fast" leaveFrom="leave-from" leaveTo="leave-to">
I am fast
<div>I am fast</div>
</Transition.Child>
<Transition.Child leave="leave-slow" leaveFrom="leave-from" leaveTo="leave-to">
I am slow
<div>I am slow</div>
</Transition.Child>
</Transition>
@@ -781,11 +817,11 @@ describe('Transitions', () => {
<Transition.Child leave="leave-fast" leaveFrom="leave-from" leaveTo="leave-to">
<span>I am fast</span>
<Transition show={show} leave="leave-slow">
I am my own root component and I don't talk to the parent
<div>I am my own root component and I don't talk to the parent</div>
</Transition>
</Transition.Child>
<Transition.Child leave="leave-slow" leaveFrom="leave-from" leaveTo="leave-to">
I am slow
<div>I am slow</div>
</Transition.Child>
</Transition>
@@ -953,12 +989,22 @@ describe('Events', () => {
<style>{`.child-2-2.leave { transition-duration: ${leaveDuration * 2.5}ms; }`}</style>
<Transition show={show} {...getProps('root')}>
<Transition.Child {...getProps('child-1')}>Child 1.</Transition.Child>
<Transition.Child {...getProps('child-2')}>
Child 2.
<Transition.Child {...getProps('child-2-1')}>Child 2.1.</Transition.Child>
<Transition.Child {...getProps('child-2-2')}>Child 2.2.</Transition.Child>
</Transition.Child>
<div>
<Transition.Child {...getProps('child-1')}>
<div>Child 1.</div>
</Transition.Child>
<Transition.Child {...getProps('child-2')}>
<div>
<div>Child 2.</div>
<Transition.Child {...getProps('child-2-1')}>
<div>Child 2.1.</div>
</Transition.Child>
<Transition.Child {...getProps('child-2-2')}>
<div>Child 2.2.</div>
</Transition.Child>
</div>
</Transition.Child>
</div>
</Transition>
<button
@@ -1104,7 +1150,9 @@ describe('Events', () => {
leave="leave-1"
leaveFrom="leave-from"
leaveTo="leave-to"
/>
>
<div />
</Transition.Child>
<Transition.Child
enter="enter-1"
enterFrom="enter-from"
packages/@headlessui-react/src/components/transition/transition.tsx
+50 -4
View File
@@ -49,6 +49,44 @@ function splitClasses(classes: string = '') {
return classes.split(/\s+/).filter((className) => className.length > 1)
}
/**
* Check if we should forward the ref to the child element or not. This is to
* prevent crashes when the `as` prop is a Fragment _and_ the component just acts
* as a state container (aka, there is no actual transition happening).
*
* E.g.:
*
* ```tsx
* <Transition show={true}>
* <Transition.Child enter="duration-100"><div>Child 1</div></Transition.Child>
* <Transition.Child enter="duration-200"><div>Child 2</div></Transition.Child>
* </Transition>
* ```
*
* In this scenario, the child components are transitioning, but the
* `Transition` parent, which is a `Fragment`, is not. So we should not forward
* the ref to the `Fragment`.
*/
function shouldForwardRef<TTag extends ElementType = typeof DEFAULT_TRANSITION_CHILD_TAG>(
props: TransitionRootProps<TTag>
) {
return (
// If we have any of the enter/leave classes
Boolean(
props.enter ||
props.enterFrom ||
props.enterTo ||
props.leave ||
props.leaveFrom ||
props.leaveTo
) ||
// If the `as` prop is not a Fragment
(props.as ?? DEFAULT_TRANSITION_CHILD_TAG) !== Fragment ||
// If we have a single child, then we can forward the ref directly
React.Children.count(props.children) === 1
)
}
interface TransitionContextValues {
show: boolean
appear: boolean
@@ -264,7 +302,7 @@ function useNesting(done?: () => void, parent?: NestingContextValues) {
// ---
let DEFAULT_TRANSITION_CHILD_TAG = 'div' as const
let DEFAULT_TRANSITION_CHILD_TAG = Fragment
type TransitionChildRenderPropArg = MutableRefObject<HTMLDivElement>
let TransitionChildRenderFeatures = RenderFeatures.RenderStrategy
@@ -292,7 +330,9 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
...rest
} = props as typeof props
let container = useRef<HTMLElement | null>(null)
let transitionRef = useSyncRefs(container, ref)
let requiresRef = shouldForwardRef(props)
let transitionRef = useSyncRefs(...(requiresRef ? [container, ref] : ref === null ? [] : [ref]))
let strategy = rest.unmount ?? true ? RenderStrategy.Unmount : RenderStrategy.Hidden
let { show, appear, initial } = useTransitionContext()
@@ -342,10 +382,12 @@ function TransitionChildFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_
let ready = useServerHandoffComplete()
useIsoMorphicEffect(() => {
if (!requiresRef) return
if (ready && state === TreeStates.Visible && container.current === null) {
throw new Error('Did you forget to passthrough the `ref` to the actual DOM node?')
}
}, [container, state, ready])
}, [container, state, ready, requiresRef])
// Skipping initial transition
let skip = initial && !appear
@@ -495,7 +537,11 @@ function TransitionRootFn<TTag extends ElementType = typeof DEFAULT_TRANSITION_C
// @ts-expect-error
let { show, appear = false, unmount = true, ...theirProps } = props as typeof props
let internalTransitionRef = useRef<HTMLElement | null>(null)
let transitionRef = useSyncRefs(internalTransitionRef, ref)
let requiresRef = shouldForwardRef(props)
let transitionRef = useSyncRefs(
...(requiresRef ? [internalTransitionRef, ref] : ref === null ? [] : [ref])
)
// The TransitionChild will also call this hook, and we have to make sure that we are ready.
useServerHandoffComplete()