added download options for 3d plot #49
Conversation
There was a problem hiding this comment.
I got an error when I tried switch to the 3D tab for a steady state simulation because the minZ/maxZ settings didn't exist on my save.
I had to add this to src/features/saving.ts to fix it on line 70:
const data = (event.target as IDBRequest).result;
data.graphSettings = {
...defaultGraphSettings,
...data.graphSettings,
}
resolve(data);The features work well. I added some notes for refactors that can be made to simplify some of the code.
|
|
||
| const downloadName = `3D Plot of ${workspaceName}`; | ||
|
|
||
| const getPlotOptions = () => { |
There was a problem hiding this comment.
Can this be extracted outside the component so it can be used in both DownloadPlot3DButton and SeriesLineChart3D? Similar to src/app/results/generatePlotParameters.
| // Force another resize to ensure axis titles render | ||
| chart.resize(); | ||
| // eslint-disable-next-line testing-library/render-result-naming-convention | ||
| const canvas = chart.getRenderedCanvas(); |
There was a problem hiding this comment.
getRenderedCanvas is deprecated. The suggested fix is to use renderToCanvas. But is there any reason to do it this way instead of how it was done for the 2D plots in DownloadPlotButton.tsx? This does seem more robust so maybe we can switch to this method for the 2D plots as well.
| Elasticities: { x: "Species", y: "Reaction", z: "Elasticity" }, | ||
| }; | ||
|
|
||
| const getPlotOptions = () => { |
There was a problem hiding this comment.
Same note about extracting this as with the one in DownloadPlot3DButton.
| }; | ||
|
|
||
| const [item, setItem] = useState<Item>("Jacobian"); | ||
| const colorSchemeOptions = Object.fromEntries( |
There was a problem hiding this comment.
Can be moved outside the component, similar to ITEM_OPTIONS
| const [graphSettings, setGraphSettings] = useAtom(graphSettingsAtom); | ||
| const [item, setItem] = useAtom(steadyState3DItemAtom); | ||
|
|
||
| const handleZAxisChangeFor = ( |
There was a problem hiding this comment.
| const handleZAxisChangeFor = ( | |
| const handleChangeFor = ( |
| <RadixDropdownMenu.Item | ||
| className={styles.item} | ||
| onSelect={(event) => { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
I prefer it closing when you click on an option. Matches dropdown menu user experience in other apps.
| minY: number; | ||
| maxY: number; | ||
|
|
||
| isAutoscaledZ: boolean; |
There was a problem hiding this comment.
I think this can either be placed in SteadyState3DGraphSettings atom/type or renamed to isSteadyState3DAutoscaledZ just so it doesn't clash with the 3d line chart. Same goes for the other properties you added.
| isAutoscaledZ?: boolean; | ||
| minZ?: number; | ||
| maxZ?: number; | ||
| colorScheme?: Exclude<Palette, "Custom">; |
There was a problem hiding this comment.
Better to require them to exist in my opinion.
| isAutoscaledZ?: boolean; | |
| minZ?: number; | |
| maxZ?: number; | |
| colorScheme?: Exclude<Palette, "Custom">; | |
| isAutoscaledZ: boolean; | |
| minZ: number; | |
| maxZ: number; | |
| colorScheme: Exclude<Palette, "Custom">; |
| x, | ||
| y, | ||
| z, | ||
| isAutoscaledZ = true, |
There was a problem hiding this comment.
| isAutoscaledZ = true, | |
| isAutoscaledZ, |
| isAutoscaledZ = true, | ||
| minZ, | ||
| maxZ, | ||
| colorScheme = "BlueRed", |
There was a problem hiding this comment.
| colorScheme = "BlueRed", | |
| colorScheme, |
was unable to rotate the z axis vertical but moved in labels closer to the axes so they all fit on the downloaded images