tailwindlabs / tailwindui-issues

Bug fixes and feature request tracking for Tailwind UI.
233 stars 4 forks source link

Catalyst: sidebar.tsx line 118 'as={link}' error #1582

Closed bookerlyio closed 3 months ago

bookerlyio commented 3 months ago

In the base catalyst components download, specifically sidebar.tsx, I get the error:

<Headless.CloseButton

as={Link}<<<< {...props} className={classes} data-current={current ? 'true' : undefined} ref={ref}

"Type 'Url | ForwardRefExoticComponent<InternalLinkProps & Omit<DetailedHTMLProps<AnchorHTMLAttributes, HTMLAnchorElement>, "ref"> & RefAttributes<...>>' is not assignable to type 'ElementType | undefined'."

I have the latest headlessui, followed the catalyst docs to the letter.. someone please help :)!

rakr commented 3 months ago

Temporary solution until it is fixed in code or documentation, in link.tsx


export const Link = React.forwardRef(function Link(
  props: { href: string } & React.ComponentPropsWithoutRef<'a'>,
  // props: LinkProps & React.ComponentPropsWithoutRef<'a'>,
  ref: React.ForwardedRef<HTMLAnchorElement>,
) ```
RobinMalfait commented 3 months ago

Hey!

This has been fixed, you can download the latest version (https://tailwindui.com/templates/catalyst/download) or apply the following changes yourself:


Let's talk about the issue first:

The issue here is that the Link component from Next.js also has an as prop for dynamic routes (see: https://github.com/vercel/next.js/blob/v9.5.2/docs/api-reference/next/link.md#dynamic-routes) which conflicts with the as prop from Headless UI (which we use to render a component as another component. E.g.: <MenuButton as={MyStyledButton} />).

To solve this, our goal is to make sure that those as prop definitions don't conflict anymore.

We now make sure to render the <Link /> component as a child of the <CloseButton /> component instead of using the as prop on the <CloseButton />.

This change looks like this (sidebar.tsx):

       {'href' in props ? (
-        <Headless.CloseButton
-          as={Link}
-          {...props}
-          className={classes}
-          data-current={current ? 'true' : undefined}
-          ref={ref}
-        >
-          <TouchTarget>{children}</TouchTarget>
+        <Headless.CloseButton as={Fragment} ref={ref}>
+          <Link className={classes} {...props} data-current={current ? 'true' : undefined
+            <TouchTarget>{children}</TouchTarget>
+          </Link>
         </Headless.CloseButton>
       ) : (
  1. The as={Link} part is removed
  2. The <Link /> is rendered as a child of the <Headless.CloseButton/>
  3. The {...props}, className and data-current props are moved to the <Link /> component.
  4. The <Headless.CloseButton /> now uses as={Fragment} to make sure that we don't render a button by default, but use the children instead.

This problem also exists in the DropdownItem component (which can be found in dropdown.tsx), you can apply the following changes:

 export function DropdownItem({
   className,
   ...props
 }: { className?: string } & (
-  | Omit<Headless.MenuItemProps<typeof Link>, 'className'>
-  | Omit<Headless.MenuItemProps<'button'>, 'className'>
+  | Omit<React.ComponentPropsWithoutRef<typeof Link>, 'className'>
+  | Omit<React.ComponentPropsWithoutRef<'button'>, 'className'>

This small change is needed because we now rely on the props of the underlying components directly instead of using the MenuItemProps from Headless UI. In the next diff you'll notice that we don't put any props on the <Headless.MenuItem> at all, so therefore we don't need to type the incoming props with this information in mind.

In that same DropdownItem, you can apply the following diff:

-  return 'href' in props ? (
-    <Headless.MenuItem as={Link} {...props} className={classes} />
-  ) : (
-    <Headless.MenuItem as="button" type="button" {...props} className={classes} />
-  )
+  return (
+    <Headless.MenuItem>
+      {'href' in props ? (
+        <Link {...props} className={classes} />
+      ) : (
+        <button type="button" {...props} className={classes} />
+      )}
+    </Headless.MenuItem>
+  )

What you will notice is that we now wrap the entire returned JSX with <Headless.MenuItem /> and use the <Link /> and <button /> as children directly. You'll also notice that the props are directly spread onto those <Link /> and <button> components which means there are no Headless UI (conflicting) props involved anymore.

If these diffs are bit too confusing, you can download the latest version (https://tailwindui.com/templates/catalyst/download) and get the code for sidebar.tsx and dropdown.tsx.

Hope this helps!

bookerlyio commented 3 months ago

Thank you, much appreciated!

ngbrown commented 1 month ago

In the fix released, the ref property from the Headless.CloseButton should have been moved to the Link component. This is because ref doesn't apply to Fragment.