Add configuration to improve large page experience #227
Conversation
Use dynamic application of the tooltipsterification process to create it, then open it, on click or hover rather than processing potentially thousands of elements during the page load event. The tooltipster docs suggest that this is a less performant strategy (I expect very small hiccups on hover), but for large pages it is much more preferable than a second-long load delay. This will be refactored later into an option
This reverts commit 8c36803.
There was a problem hiding this comment.
Thanks! I think this PR is good. I left some comments to continue the conversation, but I think we will be able to merge it soon.
Can you add a test that uses this hoverxref_tooltip_lazy=True and checks that we are including some porting of the right Javascript code? 🙏🏼
I'm pinging @agjohnson since he know more about Javascript than myself just in case he sees something that I'm not seeing.
humitos
requested review from
agjohnson
and removed request for
a team and
ericholscher
|
...github, when I click "re-request review" I do not mean "remove other reviewers" ;-; |
There was a problem hiding this comment.
Very cool! Thanks for the elegant implementation and test case!
Since Disnake community will actively be using this very useful (but optional) feature, I would assume that it will quickly be tested -- any subsequent optimizations and bug fixes will most welcome!
| addTooltip($(this)).tooltipster('open'); | ||
| }); | ||
| {% else %} | ||
| $('.{{ hoverxref_css_class_prefix }}hoverxref.external').each(function () { $(this).removeAttr('title') }); |
There was a problem hiding this comment.
Shouldn't we move this outside the if/else and put to below the comment in line 145? This code is required for both paths,.
There was a problem hiding this comment.
I think the intention is to keep the title attr until tooltipster is actually triggered, which happens "lazily".
There was a problem hiding this comment.
That is the intention. I could add a comment to add the reasoning? Editing the dom on page load to the degree found in the disnake docs (150kish titles) is a significant amount of effort that rarely if ever needs to be done in the first place (since a user usually doesn't hover over more than a few hoverxref links, anyway, and certainly not in the thousands) so deffering it with popup generation makes sense to me.
I don't currently have the measurements, but it was somewhere in the ballpark of 0.5-1.5s of delay from that line alone, which is on one hand reasonable, on the other the entire point of this feature is to remove that behavior in the first place.
|
Hi, is there any eta on when this might be merged? We're excited to begin to use this at disnake but are waiting on a sphinx-hoverxref release, and I'd like to implement this in our next stable doc release. 🙂 |
|
Hi everybody. Thanks for all your work here. I will try to take a final look this week, merge and release a new version 🤞🏼 |
|
|
|
"soon this week" = 17 minutes later 🥳 |
|
We've now deployed this change to https://docs.disnake.dev/en/latest/ 😄 |
On significantly large pages (for example, the disnake documentation at https://docs.disnake.dev/en/stable/api.html) sphinx-hoverxref's initialization makes up a very significant portion of page load time (multiple seconds) as it edits the html at load time to add tooltips.
This can be dramatically improved by lazily initializing tooltipster. I wrote some code to prove the concept and speed (visible live at https://disnake--393.org.readthedocs.build/en/393/api.html) and it is a huge improvement.
I'm definitely willing to do what's needed to bring it up to style/etc with the rest of the project. Let me know what needs to be done for it to be mergeable? It'd be lovely to get rid of the page load delay for good.
(Edit: fixed the preview link to point at correct PR, although the other pr also had it active)
📚 Documentation preview 📚: https://sphinx-hoverxref--227.org.readthedocs.build/en/227/
📚 Documentation preview 📚: https://read-the-docs-sphinx-hoverxref--227.com.readthedocs.build/en/227/