jasonkuhrt / react-popover

A smart popover component for React
600 stars 253 forks source link

Add prop to Popover API to allow the click on target element to trigger onOuterAction #94

Open dhassouni opened 7 years ago

dhassouni commented 7 years ago

It is a fairly common use case that popovers can be closed by clicking the same target element that opened it. I believe that since Popover becomes the owner of the target, they are being treated similarly here. However, we already have many users claiming it is unintuitive that clicking the same location does not close the popover (it is, after all, outside of the popover). I am suggesting a boolean prop such as targetIsOutsidePopover where the default could be false so as not to change the default behavior, but allow those of us wanted this behavior to flip the switch to have it behave as we'd like.

jasonkuhrt commented 7 years ago

In most use-cases I presume the target is setup to also toggle the popover when tapped/clicked. It sounds like in your case it isn't?

dhassouni commented 7 years ago

Sorry if I wasn't clear because I think we are saying the same thing? I would like the target to toggle the popover's open/closed state. However, with my implementation of react-popover, clicking the target element (a button) only seems to open the popover. Clicking the target again currently does not close the popover, but I would like it to do so. Does that make sense?

jasonkuhrt commented 7 years ago

@dhassouni I understand that you want target to be considered outside the popover. For prioritization purposes I just want to confirm that we agree what you are asking for is sugar. I think it is because I don't see anything about the API technically holding you back from building your desired behaviour. For example I'm pretty sure toggling the popover on target click/tap is good enough for your use-case. Its what we do the sandbox example.

dhassouni commented 7 years ago

Ah, you are right. I missed how easy this can be to do. So yes, it would be sugar. And I'd now advise that you prioritize this relatively low (if at all). Thanks for pointing me to this example.

dhassouni commented 7 years ago

Though one other consideration: The popover class is applied to the div that includes the content of the popover as well as the vertical or horizontal space taken up by the tip. In my use case, I'm passing in a tipSize of 16px for example, because I need a bigger tip to style consistently with my application. The entire vertical space that is above the content of the popover (in examples where the popover opens below) but below the top of the tip is not considered "outside". It would be nice to clean this up such that clicking outside of the popover as far as the user is concerned always triggered the onOuterAction(). I've included a small screenshot of the area that I am referring to if that helps. The area highlighted in red (minus perhaps the actual tip), should be considered outsize the popover.

screen shot 2017-03-14 at 11 26 10 am

If you agree this is something that can and should be addressed, we can create a separate issue for it so as not to conflate the title of this one with something a little bit different.