Fix plot progress bar #4781
Merged
Fix plot progress bar #4781
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Show progress bar at top of editor Remove old progress bars before new render
isabelizimm
approved these changes
Sep 23, 2024
seeM
reviewed
Sep 24, 2024
There was a problem hiding this comment.
Just a small comment on the code since @isabelizimm already verified the behavior.
| // Before starting a new render, remove any existing progress bars. This prevents | ||
| // a buildup of progress bars when rendering multiple times and ensures the progress bar | ||
| // is removed when a new render is requested before the previous one completes. | ||
| progressRef.current.replaceChildren(); |
There was a problem hiding this comment.
Should we maintain a single progress bar and ensure that done()/hide() are called at the correct times rather than deleting and recreating the DOM nodes? This seems fine as is though.
There was a problem hiding this comment.
I think this might be more complicated with the timing to create the progress bar once the ref to the parent div is available. I'll see if I can follow up with something if it isn't too complicated.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Address #4562
Add a flex direction on the plot element so that the children are in a single column.
There was another issue with leftover progress bars that's been fixed by removing any existing ones before creating a new one. Sometimes this would leave a progress bar in perpetual animation when another render is called before the current one completes (triggered by resizing the plot container when a render is in progress). It looked even worse when another render is called and it becomes difficult to distinguish the current progress bar with a previous one.
Before:
After:
Screen.Recording.2024-09-23.at.9.04.35.AM.mov
QA Notes
The feature flag to enable plots in editors has changed recently and reset the flag. It may need to be re-enabled.
Viewing a plot in an editor would render the progress bar in the middle, behind the plot image. Trigger a render by resizing the editor or changing the sizing policy in the Plots View.
Resizing a plot in editor or the Plots View would create another progress bar. If a render is triggered while one is already in progress, the old progress bar may not be set to done so it looks like a render is always in progress.