-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
refactor(tooltip): migrate from wz_tooltip to Floating UI #1429
refactor(tooltip): migrate from wz_tooltip to Floating UI #1429
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This PR is now ready for review, and I have updated the status and rewritten the description to reflect the changes here. Also, this PR contains changes from #1477, so there is some noise here that otherwise won't be when that (now approved) PR is ultimately merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems functional. I did not review any of the code changes outside of the php files.
I have been maintaining this PR for over 10 weeks. It has received one approval. My understanding is the solution that has been built here aligns with the goals of the project. Is there any intent for this work to be merged? If not, I will close the PR. |
To be fair, it was a draft until Apr 17, so it's only been stagnant for about five weeks, not "over ten". And there are many things that have been open for two weeks or more (including some of mine) simply due to there being bottlenecks in the process. I got burnout from the last release, so I'm trying to timebox myself with this one. I approved based on functional testing, but I don't really want to merge without @luchaos's approval since it's a framework change, and he's really in charge of the ecosystem. |
Fair enough @Jamiras, you are right. I appreciate your perspective. |
@luchaos @Jamiras This branch has been updated to resolve all outstanding conflicts with V3. Note: after resolving the merge conflicts, I made a slight alteration to the tooltip fade-out animation so they more closely (if not identically) match what's on prod today. This was a simple matter of removing a 95% scale CSS effect. The change happened in this commit: 3cda5af |
@luchaos Is there any way I can get your review and/or approval on this PR? I don't want to go making any changes or fixes to tooltips until wz_tooltip is removed from the codebase, and this PR does that. The idea is to get tooltips to have functional parity with what's in prod today, but implemented with the latest version of Popper.js (Floating UI). I've been maintaining this PR for over 10 weeks and I'm beginning to wonder if it's not getting a review because there's a disagreement with the technology choice here. If that's the case, that's okay -- but please tell me! I can do whatever rework is necessary to satisfy the project's goals. Based on chatter in the workshop server my understanding was moving to the latest version of Popper.js was our intent for tooltips, and that is accomplished here. Thanks! |
Thank you very much for taking care of this and cleaning up a lot of other areas while at it! |
Generally, as long as you have an approval from someone else, it's safe to merge. In this case, I approved it a while ago, but left it open because I was specifically looking for luchaos's approval. Since he has now given it, I see no reason why it can't be merged.
You should always test anything you think might be affected by the change. As far as I know, these tooltips are only used for the various avatar popups, which all appear to be working as expected. The only other thing I can think of is cross-browser support. I'm assuming since this is an external package they'll have done whatever is necessary for support on Chrome/Firefox/Edge/Safari Windows/Linux/iOS/Android etc. |
absolutely, floating UI has wide support of modern browsers in mind: https://floating-ui.com/docs/misc |
This PR refactors the site's tooltip implementation away from wz_tooltip. Tooltips are now implemented using Floating UI (the successor to popperjs).
Floating UI Docs
Why?
How?
All wz_tooltip code, including its plugins, has been removed.
Floating UI tooltip code is implemented with TypeScript in resources/js/tooltip. I have also added numerous tests for our code that uses the library. Most of the code additions in this PR is actually just for test cases.
The intent is for tooltips to remain as close to functionally equivalent as possible to the existing implementation. This is meant to be a refactor, not an enhancement.
Therefore:
x-init
directive.Bundle size difference:
app.js before: ~23Kb gzipped
app.js now: ~29Kb gzipped
The bundle now appears larger, however this metric is misleading. wz_tooltip is currently being loaded directly in the document body on prod, and it is ~15Kb gzipped. In total, this change actually achieves a small payload savings.
Please note that in this video my cursor is being slightly smoothed by my screen recording software, thus the tooltip position appears to be inaccurate, but this is not indicative of reality.
New.Recording.Apr.17.2023.0613.PM.mp4