From f3885af0a09e1fe2519881d3b1cc41afa1a4defe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Gr=C3=A9lard?= Date: Wed, 21 Feb 2024 10:11:23 +0000 Subject: [PATCH 1/3] Simplify complex `asChild` scenarios and fix reconciliation issues --- .../app/test-tabnav/(accounts)/page.tsx | 5 + .../app/test-tabnav/documents/page.tsx | 5 + apps/playground/app/test-tabnav/layout.tsx | 25 +++++ apps/playground/app/test-tabnav/nav.tsx | 57 ++++++++++ .../app/test-tabnav/settings/page.tsx | 5 + .../radix-ui-themes/src/components/avatar.tsx | 32 +++--- .../src/components/container.tsx | 27 ++--- .../src/components/scroll-area.tsx | 106 +++++++++--------- .../src/components/tab-nav.tsx | 27 +++-- .../first-child-might-adopt-subtree.ts | 21 ++++ .../radix-ui-themes/src/helpers/get-root.tsx | 49 -------- packages/radix-ui-themes/src/helpers/index.ts | 2 +- 12 files changed, 208 insertions(+), 153 deletions(-) create mode 100644 apps/playground/app/test-tabnav/(accounts)/page.tsx create mode 100644 apps/playground/app/test-tabnav/documents/page.tsx create mode 100644 apps/playground/app/test-tabnav/layout.tsx create mode 100644 apps/playground/app/test-tabnav/nav.tsx create mode 100644 apps/playground/app/test-tabnav/settings/page.tsx create mode 100644 packages/radix-ui-themes/src/helpers/first-child-might-adopt-subtree.ts delete mode 100644 packages/radix-ui-themes/src/helpers/get-root.tsx diff --git a/apps/playground/app/test-tabnav/(accounts)/page.tsx b/apps/playground/app/test-tabnav/(accounts)/page.tsx new file mode 100644 index 00000000..7888ce51 --- /dev/null +++ b/apps/playground/app/test-tabnav/(accounts)/page.tsx @@ -0,0 +1,5 @@ +import { Heading } from '@radix-ui/themes'; + +export default function Accounts() { + return Accounts; +} diff --git a/apps/playground/app/test-tabnav/documents/page.tsx b/apps/playground/app/test-tabnav/documents/page.tsx new file mode 100644 index 00000000..c37d1d33 --- /dev/null +++ b/apps/playground/app/test-tabnav/documents/page.tsx @@ -0,0 +1,5 @@ +import { Heading } from '@radix-ui/themes'; + +export default function Documents() { + return Documents; +} diff --git a/apps/playground/app/test-tabnav/layout.tsx b/apps/playground/app/test-tabnav/layout.tsx new file mode 100644 index 00000000..89cf1f67 --- /dev/null +++ b/apps/playground/app/test-tabnav/layout.tsx @@ -0,0 +1,25 @@ +import * as React from 'react'; +import { Theme, Container, Section, Box } from '@radix-ui/themes'; +import { NextThemeProvider } from '../next-theme-provider'; +import { Nav } from './nav'; + +export default function Test({ children }: { children: React.ReactNode }) { + return ( + + + + +
+ +
+
+ +
+ + + + + ); +} diff --git a/apps/playground/app/test-tabnav/nav.tsx b/apps/playground/app/test-tabnav/nav.tsx new file mode 100644 index 00000000..1d195712 --- /dev/null +++ b/apps/playground/app/test-tabnav/nav.tsx @@ -0,0 +1,57 @@ +'use client'; + +import NextLink from 'next/link'; +import { usePathname } from 'next/navigation'; +import { TabNavRoot, TabNavLink, Heading, Flex, Text } from '@radix-ui/themes'; + +export function Nav() { + const pathname = usePathname(); + return ( + + + Straight up `TabNavLink` + + + Accounts + + + Documents + + + Settings + + + + + + {``} with `NextLink` + + + Accounts + + + Documents + + + Settings + + + + + + {``} with `TabNavLink` + + + Accounts + + + Documents + + + Settings + + + + + ); +} diff --git a/apps/playground/app/test-tabnav/settings/page.tsx b/apps/playground/app/test-tabnav/settings/page.tsx new file mode 100644 index 00000000..c86dd578 --- /dev/null +++ b/apps/playground/app/test-tabnav/settings/page.tsx @@ -0,0 +1,5 @@ +import { Heading } from '@radix-ui/themes'; + +export default function Settings() { + return Settings; +} diff --git a/packages/radix-ui-themes/src/components/avatar.tsx b/packages/radix-ui-themes/src/components/avatar.tsx index ce900bfd..8c43504f 100644 --- a/packages/radix-ui-themes/src/components/avatar.tsx +++ b/packages/radix-ui-themes/src/components/avatar.tsx @@ -4,7 +4,7 @@ import * as React from 'react'; import classNames from 'classnames'; import * as AvatarPrimitive from '@radix-ui/react-avatar'; import { avatarPropDefs } from './avatar.props.js'; -import { extractProps, getRoot } from '../helpers/index.js'; +import { extractProps, firstChildMightAdoptSubtree } from '../helpers/index.js'; import { marginPropDefs } from '../props/index.js'; import type { ComponentPropsWithoutColor } from '../helpers/index.js'; @@ -12,32 +12,26 @@ import type { MarginProps, GetPropDefTypes } from '../props/index.js'; interface AvatarProps extends MarginProps, AvatarImplProps {} const Avatar = React.forwardRef((props, forwardedRef) => { - const { - asChild, - children: childrenProp, - className, - style, - color, - radius, - ...imageProps - } = extractProps(props, avatarPropDefs, marginPropDefs); - - const { Root: AvatarRoot } = getRoot({ - asChild, - children: childrenProp, - parent: AvatarPrimitive.Root, - }); + const { asChild, children, className, style, color, radius, ...imageProps } = extractProps( + props, + avatarPropDefs, + marginPropDefs + ); return ( // TODO as a rule, should we rather spread the props on root? - - - + {firstChildMightAdoptSubtree( + { asChild, children }, + + )} + ); }); Avatar.displayName = 'Avatar'; diff --git a/packages/radix-ui-themes/src/components/container.tsx b/packages/radix-ui-themes/src/components/container.tsx index 8d5a0794..c7849ecf 100644 --- a/packages/radix-ui-themes/src/components/container.tsx +++ b/packages/radix-ui-themes/src/components/container.tsx @@ -2,7 +2,7 @@ import * as React from 'react'; import classNames from 'classnames'; import { Slot } from '@radix-ui/react-slot'; import { containerPropDefs } from './container.props.js'; -import { extractProps, getRoot } from '../helpers/index.js'; +import { extractProps, firstChildMightAdoptSubtree } from '../helpers/index.js'; import { deprecatedLayoutPropDefs, heightPropDefs, @@ -23,12 +23,7 @@ interface ContainerProps ContainerOwnProps {} const Container = React.forwardRef( ({ width, minWidth, maxWidth, ...props }, forwardedRef) => { - const { - asChild, - children: childrenProp, - className, - ...containerProps - } = extractProps( + const { asChild, children, className, ...containerProps } = extractProps( props, containerPropDefs, layoutPropDefs, @@ -42,22 +37,20 @@ const Container = React.forwardRef( heightPropDefs ); - const { Root: ContainerRoot, children } = getRoot({ - asChild, - children: childrenProp, - parent: asChild ? Slot : 'div', - }); + const Comp = asChild ? Slot : 'div'; return ( - -
- {children} -
- + {firstChildMightAdoptSubtree({ asChild, children }, (children) => ( +
+ {children} +
+ ))} + ); } ); diff --git a/packages/radix-ui-themes/src/components/scroll-area.tsx b/packages/radix-ui-themes/src/components/scroll-area.tsx index 0535a137..911c229e 100644 --- a/packages/radix-ui-themes/src/components/scroll-area.tsx +++ b/packages/radix-ui-themes/src/components/scroll-area.tsx @@ -8,8 +8,8 @@ import { extractMarginProps, getMarginStyles, getResponsiveClassNames, - getRoot, mergeStyles, + firstChildMightAdoptSubtree, } from '../helpers/index.js'; import type { ComponentPropsWithoutColor } from '../helpers/index.js'; @@ -28,7 +28,7 @@ const ScrollArea = React.forwardRef((props, const { asChild, - children: childrenProp, + children, className, style, type, @@ -40,66 +40,66 @@ const ScrollArea = React.forwardRef((props, ...viewportProps } = marginRest; - const { Root: ScrollAreaRoot, children } = getRoot({ - asChild, - children: childrenProp, - parent: ScrollAreaPrimitive.Root, - }); - return ( - - - {children} - -
+ {firstChildMightAdoptSubtree({ asChild, children }, (children) => ( + <> + + {children} + + +
- {scrollbars !== 'vertical' ? ( - - - - ) : null} + {scrollbars !== 'vertical' ? ( + + + + ) : null} - {scrollbars !== 'horizontal' ? ( - - - - ) : null} + {scrollbars !== 'horizontal' ? ( + + + + ) : null} - {scrollbars === 'both' ? ( - - ) : null} - + {scrollbars === 'both' ? ( + + ) : null} + + ))} + ); }); ScrollArea.displayName = 'ScrollArea'; diff --git a/packages/radix-ui-themes/src/components/tab-nav.tsx b/packages/radix-ui-themes/src/components/tab-nav.tsx index 70002e6c..fb6dacf5 100644 --- a/packages/radix-ui-themes/src/components/tab-nav.tsx +++ b/packages/radix-ui-themes/src/components/tab-nav.tsx @@ -4,7 +4,7 @@ import * as React from 'react'; import classNames from 'classnames'; import * as NavigationMenu from '@radix-ui/react-navigation-menu'; import { tabNavLinkPropDefs, tabNavPropDefs } from './tab-nav.props.js'; -import { extractProps, getRoot } from '../helpers/index.js'; +import { extractProps, firstChildMightAdoptSubtree } from '../helpers/index.js'; import { marginPropDefs } from '../props/index.js'; import type { ComponentPropsWithoutColor } from '../helpers/index.js'; @@ -46,27 +46,26 @@ interface TabNavLinkProps extends Omit, 'onSelect'>, TabNavLinkOwnProps {} const TabNavLink = React.forwardRef((props, forwardedRef) => { - const { asChild, className, children: childrenProp, ...linkProps } = props; - - const { Root: TabNavLinkRoot, children } = getRoot({ - asChild, - children: childrenProp, - parent: NavigationMenu.Link, - }); + const { asChild, children, className, ...linkProps } = props; return ( - {}} + asChild={asChild} > - {children} - - {children} - - + {firstChildMightAdoptSubtree({ asChild, children }, (children) => ( + <> + {children} + + {children} + + + ))} + ); }); diff --git a/packages/radix-ui-themes/src/helpers/first-child-might-adopt-subtree.ts b/packages/radix-ui-themes/src/helpers/first-child-might-adopt-subtree.ts new file mode 100644 index 00000000..c950800b --- /dev/null +++ b/packages/radix-ui-themes/src/helpers/first-child-might-adopt-subtree.ts @@ -0,0 +1,21 @@ +import * as React from 'react'; + +/** + * This is a helper function that is used when a component supports `asChild` + * using the `Slot` component but its implementation contains nested DOM elements. + * + * Using it ensures if a consumer uses the `asChild` prop, the elements are in + * correct order in the DOM, adopting the intended consumer `children`. + */ +export function firstChildMightAdoptSubtree( + options: { asChild: boolean | undefined; children: React.ReactNode }, + content: React.ReactNode | ((children: React.ReactNode) => React.ReactNode) +) { + const { asChild, children } = options; + if (!asChild) return typeof content === 'function' ? content(children) : content; + + const firstChild = React.Children.only(children) as React.ReactElement; + return React.cloneElement(firstChild, { + children: typeof content === 'function' ? content(firstChild.props.children) : content, + }); +} diff --git a/packages/radix-ui-themes/src/helpers/get-root.tsx b/packages/radix-ui-themes/src/helpers/get-root.tsx deleted file mode 100644 index 15b43eef..00000000 --- a/packages/radix-ui-themes/src/helpers/get-root.tsx +++ /dev/null @@ -1,49 +0,0 @@ -import * as React from 'react'; -import { Slot } from '@radix-ui/react-slot'; - -interface GetChildrenArgs { - asChild: boolean | undefined; - children: React.ReactNode; - parent: T; -} - -export const getRoot = < - T extends React.ComponentType | keyof JSX.IntrinsicElements, - U extends React.ComponentProps extends { asChild?: boolean } ? T : Exclude ->({ - asChild, - children, - parent: Parent, -}: GetChildrenArgs): { - Root: U; - children: React.ReactNode; -} => { - if (asChild) { - let child = React.Children.only(children) as React.ReactElement; - const grandChildren = child.props.children; - return { - Root: ((props) => { - child = React.cloneElement(child, { - children: props.children, - }); - - // Make sure we don't pass `asChild` to DOM elements - if ((Parent as unknown) === Slot) { - return {child}; - } - - return ( - - {child} - - ); - }) as U, - children: grandChildren, - }; - } - - return { - Root: Parent as unknown as U, - children: children, - }; -}; diff --git a/packages/radix-ui-themes/src/helpers/index.ts b/packages/radix-ui-themes/src/helpers/index.ts index 8197e388..d20c652b 100644 --- a/packages/radix-ui-themes/src/helpers/index.ts +++ b/packages/radix-ui-themes/src/helpers/index.ts @@ -2,10 +2,10 @@ export * from './component-props-as.js'; export * from './component-props-without-color.js'; export * from './extract-margin-props.js'; export * from './extract-props.js'; +export * from './first-child-might-adopt-subtree.js'; export * from './get-margin-styles.js'; export * from './get-matching-gray-color.js'; export * from './get-responsive-styles.js'; -export * from './get-root.js'; export * from './has-own-property.js'; export * from './input-attributes.js'; export * from './is-responsive-object.js'; From 0c791680e34f8b6d55c85bcb268907e8de0dc3bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Gr=C3=A9lard?= Date: Wed, 21 Feb 2024 10:13:30 +0000 Subject: [PATCH 2/3] Update card.tsx --- packages/radix-ui-themes/src/components/card.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/radix-ui-themes/src/components/card.tsx b/packages/radix-ui-themes/src/components/card.tsx index 15b5e10e..fd5dcca3 100644 --- a/packages/radix-ui-themes/src/components/card.tsx +++ b/packages/radix-ui-themes/src/components/card.tsx @@ -2,7 +2,7 @@ import * as React from 'react'; import classNames from 'classnames'; import { Slot } from '@radix-ui/react-slot'; import { cardPropDefs } from './card.props.js'; -import { extractProps, getRoot } from '../helpers/index.js'; +import { extractProps } from '../helpers/index.js'; import { marginPropDefs } from '../props/index.js'; import type { ComponentPropsWithoutColor } from '../helpers/index.js'; From 05a01dd3545e1a26bd3a21634ac5a3d13b2295d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Gr=C3=A9lard?= Date: Wed, 21 Feb 2024 11:38:18 +0000 Subject: [PATCH 3/3] Rename util --- packages/radix-ui-themes/src/components/avatar.tsx | 7 ++----- packages/radix-ui-themes/src/components/container.tsx | 4 ++-- packages/radix-ui-themes/src/components/scroll-area.tsx | 4 ++-- packages/radix-ui-themes/src/components/tab-nav.tsx | 4 ++-- .../{first-child-might-adopt-subtree.ts => get-subtree.ts} | 2 +- packages/radix-ui-themes/src/helpers/index.ts | 2 +- 6 files changed, 10 insertions(+), 13 deletions(-) rename packages/radix-ui-themes/src/helpers/{first-child-might-adopt-subtree.ts => get-subtree.ts} (94%) diff --git a/packages/radix-ui-themes/src/components/avatar.tsx b/packages/radix-ui-themes/src/components/avatar.tsx index 8c43504f..2a96581e 100644 --- a/packages/radix-ui-themes/src/components/avatar.tsx +++ b/packages/radix-ui-themes/src/components/avatar.tsx @@ -4,7 +4,7 @@ import * as React from 'react'; import classNames from 'classnames'; import * as AvatarPrimitive from '@radix-ui/react-avatar'; import { avatarPropDefs } from './avatar.props.js'; -import { extractProps, firstChildMightAdoptSubtree } from '../helpers/index.js'; +import { extractProps, getSubtree } from '../helpers/index.js'; import { marginPropDefs } from '../props/index.js'; import type { ComponentPropsWithoutColor } from '../helpers/index.js'; @@ -27,10 +27,7 @@ const Avatar = React.forwardRef((props, forwarde style={style} asChild={asChild} > - {firstChildMightAdoptSubtree( - { asChild, children }, - - )} + {getSubtree({ asChild, children }, )} ); }); diff --git a/packages/radix-ui-themes/src/components/container.tsx b/packages/radix-ui-themes/src/components/container.tsx index c7849ecf..78b2a534 100644 --- a/packages/radix-ui-themes/src/components/container.tsx +++ b/packages/radix-ui-themes/src/components/container.tsx @@ -2,7 +2,7 @@ import * as React from 'react'; import classNames from 'classnames'; import { Slot } from '@radix-ui/react-slot'; import { containerPropDefs } from './container.props.js'; -import { extractProps, firstChildMightAdoptSubtree } from '../helpers/index.js'; +import { extractProps, getSubtree } from '../helpers/index.js'; import { deprecatedLayoutPropDefs, heightPropDefs, @@ -45,7 +45,7 @@ const Container = React.forwardRef( ref={forwardedRef} className={classNames('rt-Container', className)} > - {firstChildMightAdoptSubtree({ asChild, children }, (children) => ( + {getSubtree({ asChild, children }, (children) => (
{children}
diff --git a/packages/radix-ui-themes/src/components/scroll-area.tsx b/packages/radix-ui-themes/src/components/scroll-area.tsx index 911c229e..a695b8db 100644 --- a/packages/radix-ui-themes/src/components/scroll-area.tsx +++ b/packages/radix-ui-themes/src/components/scroll-area.tsx @@ -9,7 +9,7 @@ import { getMarginStyles, getResponsiveClassNames, mergeStyles, - firstChildMightAdoptSubtree, + getSubtree, } from '../helpers/index.js'; import type { ComponentPropsWithoutColor } from '../helpers/index.js'; @@ -48,7 +48,7 @@ const ScrollArea = React.forwardRef((props, style={mergeStyles(marginCustomProperties, style)} asChild={asChild} > - {firstChildMightAdoptSubtree({ asChild, children }, (children) => ( + {getSubtree({ asChild, children }, (children) => ( <> ((props, onSelect={() => {}} asChild={asChild} > - {firstChildMightAdoptSubtree({ asChild, children }, (children) => ( + {getSubtree({ asChild, children }, (children) => ( <> {children} diff --git a/packages/radix-ui-themes/src/helpers/first-child-might-adopt-subtree.ts b/packages/radix-ui-themes/src/helpers/get-subtree.ts similarity index 94% rename from packages/radix-ui-themes/src/helpers/first-child-might-adopt-subtree.ts rename to packages/radix-ui-themes/src/helpers/get-subtree.ts index c950800b..51595eba 100644 --- a/packages/radix-ui-themes/src/helpers/first-child-might-adopt-subtree.ts +++ b/packages/radix-ui-themes/src/helpers/get-subtree.ts @@ -7,7 +7,7 @@ import * as React from 'react'; * Using it ensures if a consumer uses the `asChild` prop, the elements are in * correct order in the DOM, adopting the intended consumer `children`. */ -export function firstChildMightAdoptSubtree( +export function getSubtree( options: { asChild: boolean | undefined; children: React.ReactNode }, content: React.ReactNode | ((children: React.ReactNode) => React.ReactNode) ) { diff --git a/packages/radix-ui-themes/src/helpers/index.ts b/packages/radix-ui-themes/src/helpers/index.ts index d20c652b..3ee36f7e 100644 --- a/packages/radix-ui-themes/src/helpers/index.ts +++ b/packages/radix-ui-themes/src/helpers/index.ts @@ -2,10 +2,10 @@ export * from './component-props-as.js'; export * from './component-props-without-color.js'; export * from './extract-margin-props.js'; export * from './extract-props.js'; -export * from './first-child-might-adopt-subtree.js'; export * from './get-margin-styles.js'; export * from './get-matching-gray-color.js'; export * from './get-responsive-styles.js'; +export * from './get-subtree.js'; export * from './has-own-property.js'; export * from './input-attributes.js'; export * from './is-responsive-object.js';