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

Feature Request: Support Popup close onClick when hoverable #2034

Closed
cdaringe opened this issue Aug 31, 2017 · 4 comments
Closed

Feature Request: Support Popup close onClick when hoverable #2034

cdaringe opened this issue Aug 31, 2017 · 4 comments

Comments

@cdaringe
Copy link
Contributor

Steps

  • add a Popup with hoverable

Expected Result

  • on click within the Popup, call first my onClick handler, then propagate event to internal Popup code, which closes the Popup

Actual Result

  • on click of a hoverable Popup, Popup remains visible. To close it, I have to manually manage some state to not render Popup childern

Version

0.72.0

Testcase

https://codepen.io/cdaringe/pen/RZvwdp

@levithomason
Copy link
Member

levithomason commented Aug 31, 2017

This would be a new Portal prop. I suggest:

<Portal closeOnPortalClick />

It should default to false for backward compatibility. It would then be exposed in the Popup. I suggest:

<Popup closeOnClickInside />

How does this sound?

@cdaringe
Copy link
Contributor Author

cdaringe commented Sep 1, 2017

Yup, sounds great. There may be cases where such an effect would be undesirable when it generally is desirable. Maybe exposing a shouldClose hook that accepts a function to let the user decide would be an elegant, simple API to support all of the use cases. I may be over complicating it, but I can see that function firing on mouseout, click, etc. For instance, if my popup has a clickable submenu in it, I may want to keep it open to expand that submenu.

Just thinkin out loud :)

@cdaringe
Copy link
Contributor Author

cdaringe commented Sep 1, 2017

in retrospect, i could always just evt.stopPropagation() for those things that I don't want to auto-close the popup with. disregard my latter proposal!

@levithomason
Copy link
Member

Per the PR, a controlled component pattern solves the issue. Set open via onClick.

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

No branches or pull requests

3 participants