Save smooth plot values across sessions #4220
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
julieg18
force-pushed
the
track-smooth-plot-values
branch
from
c436daf
to
ef70d30
Compare
webview/src/plots/components/templatePlots/TemplatePlotsGrid.tsx
Outdated
Show resolved
Hide resolved
| @@ -30,6 +31,7 @@ export const ZoomablePlot: React.FC<ZoomablePlotProps> = ({ | |||
| const { data, content: spec } = useGetPlot(section, id, createdSpec) | |||
| const dispatch = useDispatch() | |||
| const currentPlotProps = useRef<VegaLiteProps>() | |||
| const onPlotViewReady = useSetupSmoothPlot(id) | |||
There was a problem hiding this comment.
[C] I think this could have a more descriptive name.
There was a problem hiding this comment.
[Q] Also, would it be a better idea to pass this in for smooth plots only instead of calling this for all zoomable plots? Unless I have missed something custom plots should never need to call useSetupSmoothPlot and having the hook floating around for all plots makes things harder to follow.
There was a problem hiding this comment.
Code for smooth plots have now been moved to a react component, which is used for template plots only.
webview/src/plots/components/templatePlots/TemplatePlotsGrid.tsx
Outdated
Show resolved
Hide resolved
webview/src/plots/components/templatePlots/TemplatePlotsGrid.tsx
Outdated
Show resolved
Hide resolved
| @@ -285,7 +297,8 @@ export class WebviewMessages { | |||
| nbItemsPerRow: this.plots.getNbItemsPerRowOrWidth( | |||
| PlotsSection.TEMPLATE_PLOTS | |||
| ), | |||
| plots | |||
| plots, | |||
| smoothPlotValues: this.plots.getSmoothPlotValues() | |||
There was a problem hiding this comment.
[Q] What was the reason for keeping this separate from the plots data?
There was a problem hiding this comment.
Didn't think of it 🤦♀️.
There was a problem hiding this comment.
Ended up keeping it separate for now so we could watch for changes to values by just watching the values instead of snapshots which would cause unnecessary checks.
| @@ -48,8 +48,9 @@ export const TemplatePlotsGrid: React.FC<TemplatePlotsGridProps> = ({ | |||
|
|
|||
| const addDisabled = useCallback( | |||
| (e: Event) => { | |||
| const disabledId = (e.currentTarget as HTMLFormElement).parentElement | |||
| ?.parentElement?.parentElement?.id | |||
| const disabledId = (e.currentTarget as HTMLFormElement).closest( | |||
There was a problem hiding this comment.
I added an extra wrapper for a ref so I changed this code to use closest instead of trying to adjust for another parentElement since this looks cleaner to me. Any reason why we weren't using closest in the first place though?
There was a problem hiding this comment.
You would need to go back and check around #2747
There was a problem hiding this comment.
Thanks for the info! Not seeing any comments on this particular line in the prs relating to this code, but my guess on why we were using parentElement is because it's technically faster since closest keeps checking up to the root. I don't think this code gets fired too often (only when user's mouse enters .vega-actions) so should be alright to use 🤔
|
Alright resolved/responded to comments and added a debounce timer for keyboard events (thanks for spotting that, @mattseddon!). Going to request reviews since the code has changed quite a bit! |
|
Code Climate has analyzed commit c2ca400 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 97.1% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2%. View more on Code Climate. |
Demo
Screen.Recording.2023-07-11.at.2.51.42.PM.mov
Part of #3223