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

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

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Oct 1, 2022

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 force-pushed the gs/tooltip-experiments-for-popper branch from 04be86d to b0ce809 Compare October 3, 2022 18:57
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
js p3 Medium priority, and does not prevent the core functionality v5
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Tooltips show in wrong position when hovered from above on Safari
2 participants