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

Improve LazyPlotlyPlot lagging on resize #265

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

WardBrian
Copy link
Collaborator

This uses the debounce option to useMeasure to make it so the plots don't get re-drawn quite so often when resizing the output view. The UI lags significantly less now when the "histograms" output is active and the slider is dragged to widen that side of the screen.

I also tweaked the visuals of the fallback, since I spent some time staring at them during testing

@WardBrian WardBrian requested a review from jsoules January 29, 2025 15:30
@jsoules
Copy link
Collaborator

jsoules commented Jan 29, 2025

In general I think this is a good idea, and I can definitely see the improved smoothness.

I'm not 100% about moving the div with the ref to inside the Suspense block--doesn't this mean that you can't start measuring until Plotly loads? It might just be my eyes tricking me, but I feel like there's an extra frame of resizing/adjustment after the load that isn't there in the current pub. (Or I might just be noticing it more because of the debounce.)

Similarly, if you look at the line plots (which... I'm not sure if/how they would be affected by the changes you've made here) in the published version they're starting a little too big and then snapping back down a few pixels, while in the PR version I'm seeing them start at half width and then expand noticeably. Is something giving them a better initial value in the published version?

Regardless, the debounce is definitely the right thing to do and you should move forward with it, I just wanted to flag what I noticed.

@WardBrian
Copy link
Collaborator Author

It does appear that jitter was because of the movement of that div in the hierarchy -- fixed

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

This looks good as-is now, but if you feel like looking into the line plots thing (or just letting me know that it's all in my head), doing so would likely represent only a minor increase in scope 😄

@WardBrian WardBrian merged commit 2e5ff6d into main Jan 29, 2025
2 checks passed
@WardBrian WardBrian deleted the improve-lazy-plotly-lag branch January 29, 2025 16:39
@jsoules
Copy link
Collaborator

jsoules commented Jan 29, 2025

(Noting that per our conversation, it would in fact represent an unacceptable increase in scope)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants