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

feat(Tooltip): implement afterShow and afterHide callbacks #899

Merged
merged 6 commits into from
Jan 12, 2023
Merged

feat(Tooltip): implement afterShow and afterHide callbacks #899

merged 6 commits into from
Jan 12, 2023

Conversation

Oupsla
Copy link
Contributor

@Oupsla Oupsla commented Jan 11, 2023

fix: #898

@gabrieljablonski
Copy link
Member

Hey, thanks for the contribution.

Since we're already here, would you mind doing afterHide as well?

@Oupsla
Copy link
Contributor Author

Oupsla commented Jan 11, 2023

@gabrieljablonski done 😄

@gabrieljablonski
Copy link
Member

Thanks. At first glance everything looks good. I'll test it out and review the PR later today.

@gabrieljablonski gabrieljablonski changed the title feat(Tooltip): implement afterShow callback feat(Tooltip): implement afterShow and afterHide callbacks Jan 11, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@gabrieljablonski gabrieljablonski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a slight issue that should probably be solved.

  1. When using afterHide on a tooltip with events={['click']}, clicking anywhere outside the anchor element will trigger a handleShow(false) call, which in turn triggers the afterHide() call, which I believe is not desirable.

  2. Similarly, when using afterHide on a tooltip with delayShow, hovering away from the tooltip before it is shown will trigger a call to afterHide().

There should be a check if the tooltip was actually being shown before calling afterHide(). The same for afterShow() should probably also be done, although I don't believe it will make a difference in practice.

@Oupsla
Copy link
Contributor Author

Oupsla commented Jan 12, 2023

By using useEffect, it seems to work nicely, tell me if I have understood correctly the bugs and if it is fixed.
Below some videos with my tests

events={['click']}

Screen.Recording.2023-01-12.at.08.42.39.mov

delayShow

Screen.Recording.2023-01-12.at.08.46.03.mov

@gabrieljablonski
Copy link
Member

By using useEffect, it seems to work nicely, tell me if I have understood correctly the bugs and if it is fixed.

This fixed the problems I mentioned, but it still left the issue of the useEffect() being called on the first render.

There were a few other ways to fix this, but I ended up adding a wasShowing ref to ensure the callbacks are only called when show actually changes value.

@gabrieljablonski
Copy link
Member

Everything should be good to go. As soon as @danielbarion reviews this, I'll merge it and do the v5.5.0 release.

@danielbarion
Copy link
Member

The code looks good to me.

Thanks for the implementation @Oupsla!

Thanks, @gabrieljablonski, and feel free to merge when you want.

@gabrieljablonski gabrieljablonski merged commit f49fe16 into ReactTooltip:master Jan 12, 2023
@Oupsla Oupsla deleted the feat/afterShow branch January 13, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT REQ] afterShow callback
3 participants