[Slot] Fix children expectation when using Slottable (radix-ui#1376)

* [Slot] Fix children expectation when using Slottable Fixes radix-ui#1261 * Fix regression caught by Chromatic * Add tests * Replace with snapshot tests * PR feedback
luisorbaiceta · Jul 21, 2022 · ec106c3 · ec106c3
1 parent 5a6ee45
commit ec106c3
Show file tree

Hide file tree

Showing 5 changed files with 241 additions and 13 deletions.
diff --git a/.yarn/versions/10369624.yml b/.yarn/versions/10369624.yml
@@ -0,0 +1,43 @@
+releases:
+  "@radix-ui/react-accessible-icon": patch
+  "@radix-ui/react-accordion": patch
+  "@radix-ui/react-alert-dialog": patch
+  "@radix-ui/react-announce": patch
+  "@radix-ui/react-arrow": patch
+  "@radix-ui/react-aspect-ratio": patch
+  "@radix-ui/react-avatar": patch
+  "@radix-ui/react-checkbox": patch
+  "@radix-ui/react-collapsible": patch
+  "@radix-ui/react-collection": patch
+  "@radix-ui/react-context-menu": patch
+  "@radix-ui/react-dialog": patch
+  "@radix-ui/react-dismissable-layer": patch
+  "@radix-ui/react-dropdown-menu": patch
+  "@radix-ui/react-focus-scope": patch
+  "@radix-ui/react-hover-card": patch
+  "@radix-ui/react-label": patch
+  "@radix-ui/react-menu": patch
+  "@radix-ui/react-navigation-menu": patch
+  "@radix-ui/react-popover": patch
+  "@radix-ui/react-popper": patch
+  "@radix-ui/react-portal": patch
+  "@radix-ui/react-primitive": patch
+  "@radix-ui/react-progress": patch
+  "@radix-ui/react-radio-group": patch
+  "@radix-ui/react-roving-focus": patch
+  "@radix-ui/react-scroll-area": patch
+  "@radix-ui/react-select": patch
+  "@radix-ui/react-separator": patch
+  "@radix-ui/react-slider": patch
+  "@radix-ui/react-slot": patch
+  "@radix-ui/react-switch": patch
+  "@radix-ui/react-tabs": patch
+  "@radix-ui/react-toast": patch
+  "@radix-ui/react-toggle": patch
+  "@radix-ui/react-toggle-group": patch
+  "@radix-ui/react-toolbar": patch
+  "@radix-ui/react-tooltip": patch
+  "@radix-ui/react-visually-hidden": patch
+
+declined:
+  - primitives
diff --git a/packages/react/slot/src/Slot.stories.tsx b/packages/react/slot/src/Slot.stories.tsx
@@ -53,6 +53,31 @@ export const WithComposedEvents = () => (
   </>
 );
 
+export const ButtonAsLink = () => (
+  <>
+    <h1>Button with left/right icons</h1>
+    <Button
+      iconLeft={<MockIcon color="tomato" />}
+      iconRight={<MockIcon color="royalblue" />}
+      ref={console.log}
+    >
+      Button <em>text</em>
+    </Button>
+
+    <h1>Button with left/right icons as link (asChild)</h1>
+    <Button
+      asChild
+      iconLeft={<MockIcon color="tomato" />}
+      iconRight={<MockIcon color="royalblue" />}
+      ref={console.log}
+    >
+      <a href="https://radix-ui.com">
+        Button <em>text</em>
+      </a>
+    </Button>
+  </>
+);
+
 export const Chromatic = () => (
   <>
     <h1>Without Slottable</h1>
@@ -184,6 +209,22 @@ export const Chromatic = () => (
     <ErrorBoundary>
       <SlotWithSlottable>{false}</SlotWithSlottable>
     </ErrorBoundary>
+
+    <h2>Button with left/right icons</h2>
+    <Button iconLeft={<MockIcon color="tomato" />} iconRight={<MockIcon color="royalblue" />}>
+      Button <em>text</em>
+    </Button>
+
+    <h2>Button with left/right icons as link (asChild)</h2>
+    <Button
+      asChild
+      iconLeft={<MockIcon color="tomato" />}
+      iconRight={<MockIcon color="royalblue" />}
+    >
+      <a href="https://radix-ui.com">
+        Button <em>text</em>
+      </a>
+    </Button>
   </>
 );
 Chromatic.parameters = { chromatic: { disable: false } };
@@ -250,3 +291,52 @@ const SlotWithoutPreventableEvent = (props: any) => (
     }}
   />
 );
+
+const Button = React.forwardRef<
+  React.ElementRef<'button'>,
+  React.ComponentProps<'button'> & {
+    asChild?: boolean;
+    iconLeft?: React.ReactNode;
+    iconRight?: React.ReactNode;
+  }
+>(({ children, asChild = false, iconLeft, iconRight, ...props }, forwardedRef) => {
+  const Comp = asChild ? Slot : 'button';
+  return (
+    <Comp
+      {...props}
+      ref={forwardedRef}
+      style={{
+        display: 'inline-flex',
+        alignItems: 'center',
+        gap: 5,
+        border: '1px solid black',
+        padding: 10,
+        backgroundColor: 'white',
+        fontFamily: 'apple-system, BlinkMacSystemFont, helvetica, arial, sans-serif',
+        fontSize: 14,
+        borderRadius: 3,
+        ...props.style,
+      }}
+    >
+      {iconLeft}
+      <Slottable>{children}</Slottable>
+      {iconRight}
+    </Comp>
+  );
+});
+
+const MockIcon = React.forwardRef<React.ElementRef<'span'>, React.ComponentProps<'span'>>(
+  ({ color = 'tomato', ...props }, forwardedRef) => (
+    <span
+      ref={forwardedRef}
+      {...props}
+      style={{
+        display: 'inline-block',
+        width: 10,
+        height: 10,
+        backgroundColor: color,
+        ...props.style,
+      }}
+    />
+  )
+);
diff --git a/packages/react/slot/src/Slot.test.tsx b/packages/react/slot/src/Slot.test.tsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import { render, screen, fireEvent } from '@testing-library/react';
-import { Slot } from '@radix-ui/react-slot';
+import { Slot, Slottable } from '@radix-ui/react-slot';
 
 describe('given a slotted Trigger', () => {
   describe('with onClick on itself', () => {
@@ -108,6 +108,52 @@ describe('given a slotted Trigger', () => {
   });
 });
 
+describe('given a Button with Slottable', () => {
+  describe('without asChild', () => {
+    it('should render a button with icon on the left/right', async () => {
+      const tree = render(
+        <Button iconLeft={<span>left</span>} iconRight={<span>right</span>}>
+          Button <em>text</em>
+        </Button>
+      );
+
+      expect(tree.container).toMatchSnapshot();
+    });
+  });
+
+  describe('with asChild', () => {
+    it('should render a link with icon on the left/right', async () => {
+      const tree = render(
+        <Button iconLeft={<span>left</span>} iconRight={<span>right</span>} asChild>
+          <a href="https://radix-ui.com">
+            Button <em>text</em>
+          </a>
+        </Button>
+      );
+
+      expect(tree.container).toMatchSnapshot();
+    });
+  });
+});
+
 type TriggerProps = React.ComponentProps<'button'> & { as: React.ElementType };
 
 const Trigger = ({ as: Comp = 'button', ...props }: TriggerProps) => <Comp {...props} />;
+
+const Button = React.forwardRef<
+  React.ElementRef<'button'>,
+  React.ComponentProps<'button'> & {
+    asChild?: boolean;
+    iconLeft?: React.ReactNode;
+    iconRight?: React.ReactNode;
+  }
+>(({ children, asChild = false, iconLeft, iconRight, ...props }, forwardedRef) => {
+  const Comp = asChild ? Slot : 'button';
+  return (
+    <Comp {...props} ref={forwardedRef}>
+      {iconLeft}
+      <Slottable>{children}</Slottable>
+      {iconRight}
+    </Comp>
+  );
+});
diff --git a/packages/react/slot/src/Slot.tsx b/packages/react/slot/src/Slot.tsx
@@ -11,20 +11,32 @@ interface SlotProps extends React.HTMLAttributes<HTMLElement> {
 
 const Slot = React.forwardRef<HTMLElement, SlotProps>((props, forwardedRef) => {
   const { children, ...slotProps } = props;
+  const childrenArray = React.Children.toArray(children);
+  const slottable = childrenArray.find(isSlottable);
+
+  if (slottable) {
+    // the new element to render is the one passed as a child of `Slottable`
+    const newElement = slottable.props.children as React.ReactNode;
+
+    const newChildren = childrenArray.map((child) => {
+      if (child === slottable) {
+        // because the new element will be the one rendered, we are only interested
+        // in grabbing its children (`newElement.props.children`)
+        if (React.Children.count(newElement) > 1) return React.Children.only(null);
+        return React.isValidElement(newElement)
+          ? (newElement.props.children as React.ReactNode)
+          : null;
+      } else {
+        return child;
+      }
+    });
 
-  if (React.Children.toArray(children).some(isSlottable)) {
     return (
-      <>
-        {React.Children.map(children, (child) => {
-          return isSlottable(child) ? (
-            <SlotClone {...slotProps} ref={forwardedRef}>
-              {child.props.children}
-            </SlotClone>
-          ) : (
-            child
-          );
-        })}
-      </>
+      <SlotClone {...slotProps} ref={forwardedRef}>
+        {React.isValidElement(newElement)
+          ? React.cloneElement(newElement, undefined, newChildren)
+          : null}
+      </SlotClone>
     );
   }
 

diff --git a/packages/react/slot/src/__snapshots__/Slot.test.tsx.snap b/packages/react/slot/src/__snapshots__/Slot.test.tsx.snap
@@ -0,0 +1,37 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`given a Button with Slottable with asChild should render a link with icon on the left/right 1`] = `
+<div>
+  <a
+    href="https://radix-ui.com"
+  >
+    <span>
+      left
+    </span>
+    Button 
+    <em>
+      text
+    </em>
+    <span>
+      right
+    </span>
+  </a>
+</div>
+`;
+
+exports[`given a Button with Slottable without asChild should render a button with icon on the left/right 1`] = `
+<div>
+  <button>
+    <span>
+      left
+    </span>
+    Button 
+    <em>
+      text
+    </em>
+    <span>
+      right
+    </span>
+  </button>
+</div>
+`;