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

Revert isMouseOutside changes in favor of mouseleave event listener #127

Merged
merged 6 commits into from
Mar 14, 2021

Conversation

denisborovikov
Copy link
Collaborator

@denisborovikov denisborovikov commented Mar 13, 2021

The PR is an attempt to fix some edge-case bugs introduced after replacing mouseleave event listener with custom mouse outside position calculations.

Some thoughts are in discussion #126

The final fix doesn't change the layout of the tooltip in order to avoid breaking changes.

  1. mouseleave listener reverted, isMouseOutside removed
  2. New attribute added to the tooltip container, "data-popper-interactive" to display if the tooltip is interactive mode.
  3. In CSS data-popper-interactive="true" is used to set pointer-events: none to the tooltip
  4. pointer-events: none applied by default to the arrow element
  5. Fix missing dependency called clearTimeout on every render rather than on unmount, should fix Scrolling causes overflowing issues #122

@denisborovikov
Copy link
Collaborator Author

denisborovikov commented Mar 13, 2021

@czabaj @mohsinulhaq please take a look, I'll update tests and readme later. I also think we should add an idea from #126 as an example to our examples section.

Maybe we should introduce the layout/offset feature as the next major update. I still think removing the offset option and move the offset completely to CSS is a better DX than we have now. Also, it helps in interactive mode.

@denisborovikov
Copy link
Collaborator Author

denisborovikov commented Mar 13, 2021

@czabaj I'm really sorry about removing a huge chunk of code you have written. It was a good experience anyway.

@codecov
Copy link

codecov bot commented Mar 13, 2021

Codecov Report

Merging #127 (588ffd0) into master (2e6f325) will decrease coverage by 2.83%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
- Coverage   94.01%   91.17%   -2.84%     
==========================================
  Files           2        2              
  Lines         167      136      -31     
  Branches       56       41      -15     
==========================================
- Hits          157      124      -33     
- Misses         10       12       +2     
Impacted Files Coverage Δ
src/utils.ts 95.45% <ø> (-1.85%) ⬇️
src/usePopperTooltip.ts 90.35% <66.66%> (-2.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e6f325...0398e81. Read the comment docs.

@czabaj
Copy link
Contributor

czabaj commented Mar 14, 2021

@denisborovikov well, nobody likes bugs anyway - I'm open to the more bulletproof solution.

Copy link
Owner

@mohsinulhaq mohsinulhaq left a comment

Choose a reason for hiding this comment

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

Looks good to me.
And because of the pointer-events: none, we don't even need to revert back to [0, 7] offset anymore.

@denisborovikov
Copy link
Collaborator Author

denisborovikov commented Mar 14, 2021

I've updated the READMe and added a new attribute description. I think we can release this as non-breaking.

@denisborovikov denisborovikov merged commit b342cde into master Mar 14, 2021
@denisborovikov denisborovikov deleted the fix-flickering branch March 14, 2021 19:52
@yomansk8
Copy link

Hi @denisborovikov !
Thanks for the quick fix on this 🙏 Do you know when the next release including this fix will be available please ?

@mohsinulhaq
Copy link
Owner

@yomansk8 please check out v4.2.0

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.

Scrolling causes overflowing issues
4 participants