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

fix: widget persistence #535

Merged
merged 2 commits into from
Oct 17, 2024
Merged

fix: widget persistence #535

merged 2 commits into from
Oct 17, 2024

Conversation

Vtec234
Copy link
Member

@Vtec234 Vtec234 commented Oct 17, 2024

While developing some widgets today I noticed that their state is no longer persisted when the cursor moves. The resulting re-mounting/reinitialization can be bad for performance, is jittery (see recordings), and makes it impossible for widgets to animate between adjacent states.

Persistence was broken in #500 with the addition of a key that resets the entire GoalInfoDisplay component. This PR removes the key and reintroduces a state-clearing operation which is needed for correctness, but follows React docs so that there should hopefully not be any degradation in performance (I did not measure this, however). Note that setting state during rendering does technically result in a re-render, but it's a cheap one: the JSX that was returned in the first pass is never mounted or rendered.

Before: https://github.com/user-attachments/assets/4ff9c1c4-ee92-4dd0-a401-987c3259f4ca
After: https://github.com/user-attachments/assets/6e5624d0-f256-4013-8381-86421b254ccf

@Vtec234 Vtec234 requested a review from mhuisi October 17, 2024 02:58
@Vtec234
Copy link
Member Author

Vtec234 commented Oct 17, 2024

Profiling with the example from #500 reveals no measurable difference on my machine. Rendering the big term is consistently around 370ms, and there is only one slow render.

@Vtec234 Vtec234 merged commit 4ffed87 into master Oct 17, 2024
4 checks passed
@Vtec234 Vtec234 deleted the widget-persistence branch October 17, 2024 23:16
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.

1 participant