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

fix: use click outside to trigger also when previous click was on the target element #111

Closed
wants to merge 1 commit into from

Conversation

edlerd
Copy link
Contributor

@edlerd edlerd commented Nov 11, 2024

Done

This fixes and edge case, where the overlay contains a scrollbar and the user scrolls in the overlay. then clicks outside, the on close is not triggered immediately. with this fix it is triggerred on first click outside. Before the on close was only triggered on the second click outside.

See downstream bug in react-bootstrap/react-bootstrap#6836

@kyletsang
Copy link
Collaborator

I tried out this code and it didn't work.

The root cause is that waitingForTrigger.current doesn't get reset when clicking on a scrollbar because there is no click event fired.

…ng a scrollbar.

when clicking outside after interacting with the tooltips scrollbar, the on close
will not be fired when using the click event trigger. We need to use the mouseup
trigger in that case.

Signed-off-by: David Edler <[email protected]>
@edlerd
Copy link
Contributor Author

edlerd commented Nov 15, 2024

I tried out this code and it didn't work.

The root cause is that waitingForTrigger.current doesn't get reset when clicking on a scrollbar because there is no click event fired.

Right, got it. So to fix the issue, we don't need to change the implementation, but use a different root close trigger. I added an example as this is a pretty nasty edge case.

@kyletsang
Copy link
Collaborator

This is fixed in #112

We won't need the new example. Thanks for taking a look at this though!

@kyletsang kyletsang closed this Nov 19, 2024
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.

2 participants