Skip to content

Bind popper disposal with tooltip removal, trying to fix tooltip/popper inconsistencies #37235

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.

Sign up for GitHub

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

Jump to bottom
Merged
GeoSot merged 1 commit into from
Oct 6, 2022
Merged

Bind popper disposal with tooltip removal, trying to fix tooltip/popper inconsistencies #37235

GeoSot merged 1 commit into from
Oct 6, 2022

Conversation

@GeoSot
Copy link
Member

@GeoSot GeoSot commented Oct 1, 2022 •
edited
Loading

Description

According to #37068, it seems there is a case, where we initialize tooltip without popper, or we keep tooltip shown while we are destroying popper instance. The result is a tooltip without being styled

Screen Shot 2022-09-01 at 14 58 48

So I am experimenting on a continuous binding between popper instance disposal and tip element removal

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

closes #37068

@GeoSot GeoSot force-pushed the gs/tooltip-experiments-for-popper branch from 13034b1 to 04be86d Compare October 3, 2022 15:45
@GeoSot GeoSot marked this pull request as ready for review October 3, 2022 18:54
@GeoSot GeoSot requested a review from a team as a code owner October 3, 2022 18:54
@GeoSot GeoSot added the p3 Medium priority, and does not prevent the core functionality label Oct 3, 2022
@GeoSot GeoSot mentioned this pull request Oct 3, 2022
Closed
3 tasks
@GeoSot GeoSot requested a review from julien-deramond October 3, 2022 18:57
@GeoSot GeoSot force-pushed the gs/tooltip-experiments-for-popper branch from 04be86d to b0ce809 Compare October 3, 2022 18:57
julien-deramond
julien-deramond approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Haven't spotted any regressions, LGTM! I'm wondering if we couldn't add some unit tests but it might be difficult to do (to recreate the broken use case in a unit test).

@GeoSot GeoSot force-pushed the gs/tooltip-experiments-for-popper branch from b0ce809 to 313873b Compare October 6, 2022 07:16
@GeoSot
Copy link
Member Author

GeoSot commented Oct 6, 2022

I wanted to do my self too, but I wasn't able to catch the exact case. The solution came after recalling the history of tooltip (we have passed through semi-cached instance, cached instance and not cached instance) and take a higher perspective view.
I was realized that there were code blocks that didn't follow the pattern "destroy popper" => destroy element

@GeoSot GeoSot merged commit 01dc2f5 into main Oct 6, 2022
@GeoSot GeoSot deleted the gs/tooltip-experiments-for-popper branch October 6, 2022 08:31
julien-deramond pushed a commit that referenced this pull request Nov 19, 2022
@GeoSot @julien-deramond
fix tooltip/popper disposal inconsistencies (#37235)
f9e20cd
XhmikosR pushed a commit that referenced this pull request Nov 21, 2022
@GeoSot @XhmikosR
fix tooltip/popper disposal inconsistencies (#37235)
7e3074c
@charlesfries charlesfries mentioned this pull request Nov 22, 2022
Closed
3 tasks
@theborakompanioni theborakompanioni mentioned this pull request Jan 31, 2023
Merged
@cpsievert cpsievert mentioned this pull request Aug 25, 2023
Closed
@damian-snyk damian-snyk mentioned this pull request Apr 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julien-deramond julien-deramond julien-deramond approved these changes

Assignees

No one assigned

Labels

js p3 Medium priority, and does not prevent the core functionality v5

Projects

No open projects
v5.3.0
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tooltips show in wrong position when hovered from above on Safari

3 participants

@GeoSot @julien-deramond