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.

Sign up for GitHub

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

Jump to bottom

[Bug]: Popover.Button is clobbering the onClick prop #846

Closed
sebastienbarre opened this issue Oct 4, 2021 · 7 comments · Fixed by #1265
Closed

[Bug]: Popover.Button is clobbering the onClick prop #846

sebastienbarre opened this issue Oct 4, 2021 · 7 comments · Fixed by #1265

Comments

@sebastienbarre
Copy link

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v1.4.1

What browser are you using?

Chrome, Firefox, etc.

Reproduction repository

https://github.com/tailwindlabs/headlessui/blob/main/packages/%40headlessui-react/src/components/popover/popover.tsx#L475

Describe your issue

Looking at the source, it seems Popover.Button is clobbering any existing onClick prop on the button.

This prevents any code to be run before the Popover.Button's own handleClick logic.

Such code would be useful to override the Popper JS positioning logic dynamically before the popper itself is displayed, something that can't always be done with the static options passed to usePopper(). For example, I'm trying to position the popper horizontally with respect to the event.clientX, as shown by Popper JS's author here.

I believe calling any existing onClick prop at the top on handleClick, with the current event, would address this problem.

Thank you
@kb1995
Copy link

kb1995 commented Nov 15, 2021

Any updates on this? I want to add an overflow-hidden class to my body tag as I open the mobile navbar so it prevents scrolling, but not sure how to achieve this given the current behaviour

@kloh-fr
Copy link

kloh-fr commented Nov 15, 2021 •
edited
Loading

@kb1995

Any updates on this? I want to add an overflow-hidden class to my body tag as I open the mobile navbar so it prevents scrolling, but not sure how to achieve this given the current behaviour

I'm using Helmet and personally achieved this adding this in Popover.Panel:

<Helmet>
    <body className="overflow-hidden" />
</Helmet>

In case it helps. 🙂

@kb1995
Copy link

kb1995 commented Nov 15, 2021 •
edited
Loading

@kb1995

Any updates on this? I want to add an overflow-hidden class to my body tag as I open the mobile navbar so it prevents scrolling, but not sure how to achieve this given the current behaviour

I'm using Helmet and personally achieved this adding this in Popover.Panel:

<Helmet>
    <body className="overflow-hidden" />
</Helmet>

In case it helps. 🙂

@kloh-fr Thanks for the hint. I just tried it using next/head (since I don't have Helmet as a dependency) but unfortunately, it didn't work as expected.

How does your code work? Should you add the class to the wrapping body element? Wouldn't that create an empty body element?

Edit: I figured out a way to do it.

I have a <MobileMenu /> component which I pass the open prop to. Add a simple useEffect to toggle the body class based on the open state:

  useEffect(() => {
    if (open) {
      document.querySelector("body").classList.add("overflow-hidden");
    } else {
      document.querySelector("body").classList.remove("overflow-hidden");
    }
  }, [open]);

@Fxlr8
Copy link

Fxlr8 commented Dec 6, 2021

I need to open a modal right after closing the popover. The Popover.button just ignores my onClick handler. This could be fixed with an extra div wrapping the button, but this is super ugly.

@Naddiseo
Copy link

This has come up for me when integrating with nextjs's <Link> component. I'm using one of the "Stacked Layouts" from tailwindui that has mobile menu (using Disclosure.Button rather than Popover.Button but it's the same issue) I want to be able to click on one of the links without doing a full page refresh. In next, I would expect to be able to do:

navigation.map((item) =>{
  return (
               <Disclosure.Button as={Fragment} key={item.name}>
                  <Link href={item.href} >
                    <a>{item.name}</a>
                  </Link>
                </Disclosure.Button>
   );
}

However, that results in "Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()"

Next I tried moving <Link> to the outside of <Disclosure.Button>, which renders without error/warning, but the Button overrides the click handler from <Link> and does a full page refresh:

navigation.map((item) =>{
  return (
        <Link href={item.href} key={item.name} >
               <Disclosure.Button as={Fragment} >
                  
                    <a>{item.name}</a>
                
                </Disclosure.Button>
         </Link>
   );
}

Then I tried using <Disclosure as={"a"}> but that has the same problem.

I got rid of the <Disclosure.Button> altogether, but then the Popover/Disclosure doesn't close (obviously).

I think the issue is that both <Link> and <Disclosure.Button>/<Popover.Button> expect to have full control over the onClick handler of the child.

I did finally get it to sort of work by inserting a <div> between <Disclosure.Button>/<Popover.Button> and <Link>:

navigation.map((item) =>{
  return (
          <Disclosure.Button as={Fragment} key={item.name}>
                  <div className="px-4 py-4 text-base text-gray-500">
                          <Link href={item.href}>
                                <a className="block">{item.name}</a>
                         </Link>
                   </div>
        </Disclosure.Button>
   );
}

@vnugent
Copy link

vnugent commented Mar 22, 2022

I'm having a similar issue – I need to save user selections on the Popover to a global state object before closing. This hack appears to work for me:

<div onClick={() => {
              console.log('#onclick')
              doSomething() // save states or do something imperatively
            }}>
      <Popover.Button>Apply</Popover.Button>
</div>

@RobinMalfait RobinMalfait mentioned this issue Mar 22, 2022
Merged
@RobinMalfait
Copy link
Member

Hey! Thank you for your bug report!
Much appreciated! 🙏

This should be fixed by #1265, and will be available in the next release. The PR allows you to write your own event handlers like onClick, onKeyDown, ... even if we also provide those. Your handlers will be executed before ours which gives you an opportunity to do anything to the event (even event.preventDefault()) if you would like to for whatever reason.

You can already try it using npm install @headlessui/react@insiders or yarn add @headlessui/react@insiders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Properly merge incoming props tailwindlabs/headlessui
7 participants
@Naddiseo @sebastienbarre @RobinMalfait @kloh-fr @vnugent @Fxlr8 @kb1995