bug: validateDOMNesting with Dropdown component and Button child

[Ziinc]

Using the <Dropdown /> component with a button as per the examples in the docs results in the following console error:

console.error
      Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button>.
          at button
          at RenderedButton (/Users/tzeyiing/supabase/supabase/studio/node_modules/@supabase/ui/dist/cjs/components/Button/Button2.js:96:5)

seems like the implementation wraps the inner child with a button, so this:

<Dropdown {...rest}>
  <Button>...</Button>
</Dropdown>

will result in this:

<button>
  <button>...</button>
</button>

Not sure if this is expected behaviour, as docs example shows Dropdown wrapping a Button

Non-critical bug, just making issue to track.

[MildTomato]

This docs example is wrong. should be:

<Dropdown {...rest}>
  <Button as="span">...</Button>
</Dropdown>

the whole docs site will be updated with new examples. it's correct in storybook I think.

[Ziinc]

Gotcha, though if we're wrapping the dropdown children, perhaps a nicer api would be to allow button props to be passed with the Dropdown component, for example

<Dropdown buttonProps={props}>
  My text
</Dropdown>

or perhaps

<Dropdown as={Button} asProps={btnProps}>
  My text
</Dropdown>

just some thoughts 💭

[MildTomato]

Nice!

I think the main thing is I think is the trigger can be anything, and we don't dictate how the trigger should look, or what it is. So we can't assume it will always be the Button component.

The only reason I think the Dropdown wraps the trigger is so it can apply the correct aria-haspopup label to a <button>, (also so someone doesn't need to add <button> either) - but it is problematic with using a Supabase/UI <Button/> component as the trigger 😓 as you found out.

What we could do though, is maybe if we can check if the Button component is used as a trigger, then we use the radix asChild prop on the trigger, which will merge the aria-haspopup into the Button and it will stop trying to wrap a <button> tag around it. I only recently actually realised this is what asChild was useful for 😂

Think it would be a lot neater API as well if we swap the overlay prop with children, feels like that makes way more sense.
And then have a prop called trigger (which would be any JSX, or Button component) to trigger the dropdown menu appearing.

[KevinBrolly]

@Ziinc @MildTomato Trying to cut down on Github Issues, this one is two years old and does not look relevant to me anymore, can we close 🙏 ?

[MildTomato]

Yea this is no longer an issue, we have been migrating to new Dropdown component

[MildTomato]

in case anyone is wondering what the new one is consumed like...

note the trigger can be whatever you want it to be. in the example below, we use asChild to merge the trigger with the Button. but you could also not use asChild and DropdownMenuTrigger will behave like a <button>. maximum flexibility.

<DropdownMenu>
  <DropdownMenuTrigger asChild>
    <Button variant="outline">Open</Button>
  </DropdownMenuTrigger>
  <DropdownMenuContent className="w-56" align="center">
    //...
  </DropdownMenuContent>
</DropdownMenu>