atomiks / tippyjs-react

React component for Tippy.js (official)
MIT License
1.8k stars 92 forks source link

How to have "onClickOutside" ? #166

Closed CPatchane closed 4 years ago

CPatchane commented 4 years ago

Hello here πŸ‘‹

I have a specific use case where I want to display the Tippy component only on desktop views like following:

const defaultTippyProps: Omit<TippyProps, "content" | "children"> = {
  animation: "shift-away-subtle",
  arrow: true,
  interactive: true,
  trigger: "manual",
  interactiveBorder: 10,
  placement: "bottom-end",
  delay: [0, 0],
  duration: [200, 150],
  maxWidth: 500,
  appendTo: document.body,
  sticky: true,
  plugins: [sticky],
  boundary: "viewport",
  theme: "light",
  distance: 24,
  inertia: true,
}

const [showPopover, setShowPopover] = useState(false)
const open = () => setShowPopover(true)
const close = () => setShowPopover(false)

return isMobile ? (
    <>
      <MyMobileView content={content} visible={showPopover} .../>
    </>
  ) : (
    <Tippy
      content={content}
      {...defaultTippyProps}
      visible={showPopover}
      onHide={() => {
        close()
      }}
    >
      <span tabIndex={0} onClick={showPopover ? close : open}>
        {children}
      </span>
    </Tippy>
  )

The display of the tooltip depend on the showPopover state. Here are my needs:

Ideally, I should use something like onClickOutside instead of onHide to handle my use case, do you see another way to do it with the current API?

Thanks by advance and feel free to ask if you need a PR or more informations :)

atomiks commented 4 years ago

Are you able to make a CodeSandbox demo?

CPatchane commented 4 years ago

Sure :) https://codesandbox.io/s/silent-firefly-qenk4

You'll see that when you open the popover, if you resize from the mobile to desktop it keeps being opened but from desktop to mobile, it's not the case.

atomiks commented 4 years ago

I'm not sure of a good solution to this, but why do you need a different UI for mobile and desktop? As long as media queries are handled for the tippy content to adjust for small viewports, a separate component shouldn't be needed. I may be missing something πŸ˜…

In addition, in practice, resizing is very rare by real users, and needing to re-open it again from mobile to desktop is not that much of a problem imo.

CPatchane commented 4 years ago

I'm not sure of a good solution to this, but why do you need a different UI for mobile and desktop? As long as media queries are handled for the tippy content to adjust for small viewports, a separate component shouldn't be needed. I may be missing something πŸ˜…

For a product purpose, we want to have some complex popovers which look like bottom sheets on the mobile views.

In addition, in practice, resizing is very rare by real users, and needing to re-open it again from mobile to desktop is not that much of a problem imo.

I agree, it's good enough and a correct behaviour. I was just wondering if we could do it better or maybe we could have another use case for the onClickOutside need. Is this something missing on the Tippy or Popper side here?

chrisandrewca commented 4 years ago

I have a scenario where tippy is being used inside an iframe app. There's a save action invoked outside the iframe that makes its way into the app. Since the click is outside the frame, tippy doesn't automatically close. The side effect of this is that tippy will not close on its own anymore when clicking elsewhere inside the frame. So I've begun to use the visible prop. Yet, I'll need to write a document listener + element detection to perform the onClickOutside. The document bit is pretty easy but I haven't dug into the element detection. This would be a well received feature.

atomiks commented 4 years ago

@CPatchane Tippy side, but I'd generally like to avoid adding more callbacks if possible. This use case seems kind of esoteric, so if it's solvable in userland that would be preferrable

@chrisandrewca that sounds unrelated to this issue; Tippy adds mousedown listeners relative to the reference's owner document only. You can handle clicks in the main document yourself but the logic involved to handle every possible case indeed isn't that simple lol :\

I guess we could do [doc, document] as an array to handle both simultaneously

chrisandrewca commented 4 years ago

Thanks for your response and I don’t understand all the complexities. I think the problem is knowing when tippy has been clicked outside of in user code. Is there already a handler for that? If that code could take a callback, we’d know to set visible=false in our react component.

atomiks commented 4 years ago

The problem is that still wouldn't work unless we handle both documents internally I think, otherwise how can we know when to invoke an onClickOutside callback? Ideally this would "just work" without you needing to use visible.

chrisandrewca commented 4 years ago

Re iframe; I think it would be up to the app to handle clicks outside the frame and use postMessage or other infrastructure to set its internal visible state that gets passed to tippy.

Then on line 328 of that link where hide is called, a user supplied callback could be invoked for scenarios where:

It may not be a just works solution but the hook would allow us this functionality.

CPatchane commented 4 years ago

@atomiks Hi, it's me again. I still think onClickOutside could be useful, another example is that for now we can't have a Tippy fully controlled by the parent component (having a isOpen state controlling the Tippy opening state) because we can't listen the click outside event.

atomiks commented 4 years ago

@CPatchane can you make a demo of it?

CPatchane commented 4 years ago

@atomiks Sure, here it is https://codesandbox.io/s/stoic-brown-vj79h

In this example, I want to control the Tippy show/hide from the parent using renderProps, but I am stuck if I want to close the Tippy when clicking outside of it :/

atomiks commented 4 years ago

Thank you for the demo! That seems possible to fix by adding an onHide and checking if the hide was caused by the state change or an internal one like this?: https://codesandbox.io/s/focused-kirch-8gsmr

Edit: one problem I notice is that when clicking the reference while it's open, the outside mousedown hides it then it pops back open once click fires. Maybe handling your own outside click entirely is the better solution here when using React's state? https://codesandbox.io/s/jovial-feynman-dspyc

CPatchane commented 4 years ago

Thanks @atomiks for your very quick response πŸ‘

That seems possible to fix by adding an onHide and checking if the hide was caused by the state change or an internal one like this?: https://codesandbox.io/s/focused-kirch-8gsmr

I don't think it's a good solution since you keep giving some control to Tippy, but here I want the Tippy component to fully controlled by the parent, and you have the toggle bug indeed.

Maybe handling your own outside click entirely is the better solution here when using React's state? https://codesandbox.io/s/jovial-feynman-dspyc

This solution sounds kind of better to me but it make things much more complicated in the component code than just having an onClickOutside listener for example with this part:

if (
    !popper.contains(event.target) &&
    !reference.contains(event.target) &&
    !(state.isVisible && reference.contains(event.target))
  ) {
    close();
  }

It seems like I have to know how Tippy work internally with Popper/reference/state to handle it, and it could be broken easily if something internal to Tippy changes. By the way, did Tippy, in this case, still listening outside click even if it's disabled here? Here it seems like we are recreating a missing behaviour from Tippy.

atomiks commented 4 years ago

Are you able to patch node_modules with an onClickOutside hook to see if it works well with what you're trying to achieve? I wouldn't know where to put it in the listener

CPatchane commented 4 years ago

@atomiks I tried something here, it seems to pass correctly the related integration tests https://github.com/atomiks/tippyjs/pull/733

atomiks commented 4 years ago

Can you show an example of how you'd use that hook in the React demo?

CPatchane commented 4 years ago

Sure! Here it is https://github.com/atomiks/tippyjs-react/pull/178

(I got one thing to fix by doing this demo πŸ‘)

atomiks commented 4 years ago

Released, and I've added an example to the docs. Do you think we should make hideOnClick: false default if we detect the visible prop?

CPatchane commented 4 years ago

Do you think we should make hideOnClick: false default if we detect the visible prop?

It could be a good thing, I don't see a case when we want it to be true using the visible props πŸ€” On the other hand, it's always better to say explicitly than Tippy don't handle hide on click so it must at least correctly explain in the documentation