Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow <button> to be in nested components in <PopoverButton> #2715

Merged
merged 5 commits into from Aug 30, 2023

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Aug 29, 2023

We were previously checking for an HTMLElement directly but now will "pierce" the component tree if a Vue Component is used as a child of <PopoverButton> with as="template".

This resulted in us thinking the button didn't exist and clicks on it were considered outside which would close and then re-open the popover since the button being clicked was wired up to toggle it.

Basically, this didn't work before but now does:

<template>
  <Popover>
    <PopoverButton as="template">
      <ButtonWrapper>Not Working</ButtonWrapper>
    </PopoverButton>
  
    <PopoverPanel>
      <div>my content</div>
    </PopoverPanel>
  </Popover>
</template>

where ButtonWrapper is just:

<template>
  <button type="button">
    <slot />
  </button>
</template>

Since we pierce the component tree we're now able to get at the actual HTML element

Fixes #2713

The helper can currently return a component instance when it should only ever return a DOM element. So, we fix the implementation to return null if it’s not an `Element` _and_ adjust the types such that if a `ComponentPublicInstance` is passed we change the return type to `Element`.
Technically it could be an SVG element but much of Headless UI assumes HTML elements all over. So we’ll adjust the types to assume HTMLElement instead.
It doesn’t actually always return an HTMLElement but we have behavior that relies on it returning and checking for `Comment` nodes
@vercel
Copy link

vercel bot commented Aug 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2023 4:18pm
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2023 4:18pm

Copy link
Collaborator

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

We do indeed require the real underlying dom element instead of the component instance 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popover not closing on PopoverButton click
2 participants