-
Notifications
You must be signed in to change notification settings - Fork 29
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
Save comparison multi plot image values across sessions #4476
Save comparison multi plot image values across sessions #4476
Conversation
webview/src/plots/components/comparisonTable/cell/ComparisonTableMultiCell.tsx
Show resolved
Hide resolved
value: number | ||
) { | ||
if (!this.comparisonMultiPlotValues[revision]) { | ||
this.comparisonMultiPlotValues[revision] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we look into removing old revisions from the comparisonMultiPlotValues
state? Updating the state every time a revision gets removed, for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were going to do that I would check to see if the revision is in the full list of experiments/commits before removing
useEffect(() => { | ||
window.clearTimeout(changeDebounceTimer.current) | ||
changeDebounceTimer.current = window.setTimeout(() => { | ||
setComparisonMultiPlotValue(path, plot.id, currentStep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I wanted to rely solely on values
and remove useState
but that
- was less respondent (took a sec for slider change to update the image and only when the user let go of their mouse)
- made it more difficult to update the slider since the browser was handling the state instead of react
Code Climate has analyzed commit 2cab6ff and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 96.7% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.0% change). View more on Code Climate. |
Demo
Screen.Recording.2023-08-11.at.11.28.18.AM.mov
Part of #4422