From 8fa5caf0dcbcbd2ccd2ea3164e2a177a33a0aa61 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Fri, 19 Apr 2024 16:15:11 +0200 Subject: [PATCH] Change default tag from `div` to `Fragment` on `Transition` components (#3110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 `` 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 --- packages/@headlessui-react/CHANGELOG.md | 1 + .../components/disclosure/disclosure.test.tsx | 2 +- .../src/components/popover/popover.test.tsx | 2 +- .../__snapshots__/transition.test.tsx.snap | 6 - .../components/transition/transition.test.tsx | 150 ++++++++++++------ .../src/components/transition/transition.tsx | 54 ++++++- .../pages/dialog/dialog-scroll-issue.tsx | 5 +- playgrounds/react/pages/dialog/dialog.tsx | 5 +- .../react/pages/dialog/scrollable-dialog.tsx | 6 +- .../dialog/scrollable-page-with-dialog.tsx | 5 +- .../listbox/listbox-with-pure-tailwind.tsx | 3 +- .../react/pages/menu/menu-with-transition.tsx | 2 - playgrounds/react/pages/popover/popover.tsx | 3 +- .../react/pages/transitions/appear.tsx | 8 + .../component-examples/dropdown.tsx | 1 + .../transitions/component-examples/modal.tsx | 2 + .../component-examples/nested/hidden.tsx | 2 +- .../component-examples/nested/unmount.tsx | 3 +- .../component-examples/peek-a-boo.tsx | 1 + .../full-page-transition.tsx | 1 + .../layout-with-sidebar.tsx | 4 +- .../pages/transitions/react-hot-toast.tsx | 1 + 22 files changed, 182 insertions(+), 85 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 5017d60..c7de497 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Use native `useId` and `useSyncExternalStore` hooks ([#3092](https://github.com/tailwindlabs/headlessui/pull/3092)) - Use `absolute` as the default Floating UI strategy ([#3097](https://github.com/tailwindlabs/headlessui/pull/3097)) - Change default tags for `ListboxOptions`, `ListboxOption`, `ComboboxOptions`, `ComboboxOption` and `TabGroup` components ([#3109](https://github.com/tailwindlabs/headlessui/pull/3109)) +- Change default tag from `div` to `Fragment` on `Transition` components ([#3110](https://github.com/tailwindlabs/headlessui/pull/3110)) ### Added diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx index 36a5896..2c7aeb2 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx @@ -597,7 +597,7 @@ describe('Composition', () => { - + diff --git a/packages/@headlessui-react/src/components/popover/popover.test.tsx b/packages/@headlessui-react/src/components/popover/popover.test.tsx index 6fff12f..d11610a 100644 --- a/packages/@headlessui-react/src/components/popover/popover.test.tsx +++ b/packages/@headlessui-react/src/components/popover/popover.test.tsx @@ -977,7 +977,7 @@ describe('Composition', () => { - + diff --git a/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap b/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap index 1555d03..c00a201 100644 --- a/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap +++ b/packages/@headlessui-react/src/components/transition/__snapshots__/transition.test.tsx.snap @@ -111,12 +111,6 @@ exports[`Setup API shallow should passthrough all the props (that we do not use `; -exports[`Setup API shallow should render a div and its children by default 1`] = ` -
- Children -
-`; - exports[`Setup API shallow should render another component if the \`as\` prop is used and its children by default 1`] = ` Children diff --git a/packages/@headlessui-react/src/components/transition/transition.test.tsx b/packages/@headlessui-react/src/components/transition/transition.test.tsx index ac4adfa..7ce2b03 100644 --- a/packages/@headlessui-react/src/components/transition/transition.test.tsx +++ b/packages/@headlessui-react/src/components/transition/transition.test.tsx @@ -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( - +
...
) @@ -40,11 +40,6 @@ it('should render without crashing', () => { ) }) -it('should be possible to render a Transition without children', () => { - render() - 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(Children) - - 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 - Children +
Children
) @@ -99,7 +93,11 @@ describe('Setup API', () => { }) it('should render nothing when the show prop is false', () => { - let { container } = render(Children) + let { container } = render( + +
Children
+
+ ) expect(container.firstChild).toMatchSnapshot() }) @@ -115,11 +113,7 @@ describe('Setup API', () => { }) it('should be possible to use a render prop', () => { - let { container } = render( - - {() => Children} - - ) + let { container } = render({() => Children}) expect(container.firstChild).toMatchSnapshot() }) @@ -135,7 +129,7 @@ describe('Setup API', () => { expect(() => { render( - + {() => } ) @@ -153,7 +147,9 @@ describe('Setup API', () => { expect(() => { render(
- Oops + +
Oops
+
) }).toThrowErrorMatchingSnapshot() @@ -163,7 +159,9 @@ describe('Setup API', () => { it('should be possible to render a Transition.Child without children', () => { render( - + +
+ ) 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( - + +
+ ) expect(document.getElementsByClassName('transition')).not.toBeNull() @@ -182,8 +182,14 @@ describe('Setup API', () => { let { container } = render(
- Sidebar - Content +
+ +
Sidebar
+
+ +
Content
+
+
) @@ -195,8 +201,10 @@ describe('Setup API', () => { let { container } = render(
- Sidebar - Content +
+ Sidebar + Content +
) @@ -221,8 +229,10 @@ describe('Setup API', () => { let { container } = render(
- {() => } - {() =>
Content
}
+
+ {() => } + {() =>
Content
}
+
) @@ -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(
- + {() => (
- {() => } - - {() =>
Content
} -
+ {() => } + {() =>
Content
}
)}
@@ -262,8 +270,14 @@ describe('Setup API', () => { render(
- {() => Sidebar} - {() => Content} +
+ + {() => Sidebar} + + + {() => Content} + +
) @@ -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( +
+ + +
Sidebar
+
+ +
Content
+
+
+
+ ) + }).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(
- + {() => ( {() => } @@ -362,7 +398,7 @@ describe('Setup API', () => { leaveFrom="leave-from" leaveTo="leave-to" > - Children +
Children
) @@ -387,7 +423,7 @@ describe('Setup API', () => { leaveFrom="leave-from" leaveTo="leave-to" > - Children +
Children
) } @@ -732,10 +768,10 @@ describe('Transitions', () => { - I am fast +
I am fast
- I am slow +
I am slow
@@ -781,11 +817,11 @@ describe('Transitions', () => { I am fast - I am my own root component and I don't talk to the parent +
I am my own root component and I don't talk to the parent
- I am slow +
I am slow
@@ -953,12 +989,22 @@ describe('Events', () => { - Child 1. - - Child 2. - Child 2.1. - Child 2.2. - +
+ +
Child 1.
+
+ +
+
Child 2.
+ +
Child 2.1.
+
+ +
Child 2.2.
+
+
+
+
- + @@ -43,6 +43,7 @@ export default function Home() { function Box({ children }: { children?: ReactNode }) { return ( console.log('beforeEnter')} diff --git a/playgrounds/react/pages/transitions/full-page-examples/full-page-transition.tsx b/playgrounds/react/pages/transitions/full-page-examples/full-page-transition.tsx index 504e03b..1095cdc 100644 --- a/playgrounds/react/pages/transitions/full-page-examples/full-page-transition.tsx +++ b/playgrounds/react/pages/transitions/full-page-examples/full-page-transition.tsx @@ -160,6 +160,7 @@ function FullPageTransition() {
{pages.map((page, i) => ( {/* Off-canvas menu for mobile */} - + {/* Off-canvas menu overlay, show/hide based on off-canvas menu state. */} { {(t) => (