tailwindlabs / tailwindui-issues

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

Catalyst: TypeScript is not happy with `as` prop on Listbox #1612

Closed QzCurious closed 1 month ago

QzCurious commented 1 month ago

What component (if applicable)

Describe the bug TypeScript is not happy with as prop

To Reproduce Steps to reproduce the behavior:

  1. Copy paste example code
  2. Add as='div'
  3. The issue happened

Expected behavior It should be able to accept as prop like components of HeadlessUI. And also, It should be able to forward props to its single child.

Screenshots image image

Browser/Device (if applicable)

reinink commented 1 month ago

Hey there! Thanks for your patience on this one. Happy to try and help out here.

The short answer here is that the Catalyst components weren't really designed to support the as prop (other than the DropdownButton component, which if you look has the necessary generic types needed to support this).

We could, in theory, update all the Headless UI based components in Catalyst to support the as prop in the same way, but that will make these components a lot more complicated, as the as types take a bit to implement properly.

To be honest, while we like that the Headless UI components support the as prop, we don't really feel like it's necessary in the Catalyst components, as they are intended to be opinionated implementations of the Headless UI components.

Meaning, if you want a particular component to render as something other than what Catalyst ships with, feel free to update that component in your project to use that element instead, or to even add as prop support if it's really needed.

As mentioned, we opted to do this for the DropdownButton component, just because in that particular situation it's quite common to want to use an existing button component as the dropdown trigger.

That all said, I do think it's confusing that the as prop is included in the types for all of the Catalyst components — that's a bug. I am working on an update right now that removes this type from all these components.

I'll follow up here when that's been published (so you can download the components again from the Tailwind UI website), but if you want to make this change yourself it's pretty straightforward — basically just add the as prop to all the omitted Headless UI types.

For example:

  export function DialogTitle({
    className,
    ...props
- }: { className?: string } & Omit<Headless.DialogTitleProps, 'className'>) {
+ }: { className?: string } & Omit<Headless.DialogTitleProps, 'as' | 'className'>) {

I get how using the as prop can be handy in certain situations, and honestly if it had "just worked" in Catalyst without any crazy types being needed to support it, we'd be totally fine with it.

But one of our biggest goals with Catalyst is to create a component library that developers can easily read, understand, and make changes as needed for their particular project. So, we're always trying to find the right balance between "let's support all the things!!" and "let's make this code actually readable".

I hope that all makes sense! 🙏

reinink commented 1 month ago

Hey! Just an update — I've just published the updated version of Catalyst that omits the as prop from the types. If you want, you can download this latest version from the Tailwind UI website 👍

QzCurious commented 1 month ago

Thanks for sharing the thought, it helps me, as a catalyst user, to be more aligned to catalyst and/or better knowing how to customize it to my needs.