Closed iamAyushChamoli closed 6 days ago
Everything is working now :)
Glad to know! Thanks!
@jothepro ready for review
Thank you for your contribution, @iamAyushChamoli! I can confirm that this greatly reduces the amount of calls to the update function!
I am hesitant to merge this as is though, because it comes with the risk of "swallowing" events at the end of a scroll. For example, if you try scrolling really fast, for example by dragging the scrollbar all the way to the bottom of the page fast enough, the status of the TOC might not be updated correctly:
https://github.com/jothepro/doxygen-awesome-css/assets/21294002/b04c5688-dec6-4e2f-acbf-747829f65199
I don't think this is a major dealbreaker, because you need to scroll really fast to trigger the behavior, but I'd like to figure out if this can be avoided nevertheless.
@jothepro if I may suggest some fixes, How about we try modifying the time period after which the 'update()' function is throttled? Currently it is 100ms, some minor modifications like upscaling to 200ms or downscaling to 50ms might help.
Alternatively, how about batching the changes that occur during the scroll event and making the classname changes in the end?
Sure, I am open for suggestions, I am unsure on how this could best be fixed atm!
My hot take would be to delay the execution of the updates:
static throttle(func, delay) {
let lastCall = 0;
return function (...args) {
const now = new Date().getTime();
if (now - lastCall < delay) {
return;
}
lastCall = now;
return setTimeout(() => {func(...args)}, delay);
};
}
It makes the TOC updates lag by 100ms, but I don't think anybody would notice. What you think about that solution?
@jothepro Sure, I think this solution might work since we are already talking about edge case scenarios like scrolling really fast so it is very less likely that this problem will be reproduced frequently. However, if it does, the proposed solution might be a good fix at a negligible cost of TOC update by 100ms. Have you tried testing it out? If you have, can you attach a screen recording so that I can see it for myself if it is noticeable or not? (not sure, but I think you have tried coupling throttling with debouncing in this version? Anyways, if it works, it can be a good addition)
@jothepro @iamAyushChamoli Yes, fix work. I think this behavior is enough for normal work.
https://github.com/jothepro/doxygen-awesome-css/assets/29949782/177fbf29-b715-431f-a00c-61b57b64c267
Thanks @atlz253 for testing it out 🥳, @jothepro should I commit these changes?
Fixes #147
Type of change:
Code Update, calling the
update()
method indoxygen-awesome-interactive-toc.js
by throttling every 100ms instead of calling it every time the screen is scrolled.