Add export menu with SVG/PNG support and export history panel#1085
Open
Sahilkatkamwar wants to merge 1 commit intofossasia:masterfrom
Open
Add export menu with SVG/PNG support and export history panel#1085Sahilkatkamwar wants to merge 1 commit intofossasia:masterfrom
Sahilkatkamwar wants to merge 1 commit intofossasia:masterfrom
Conversation
Contributor
Reviewer's GuideImplements a custom export workflow for PlotPane by replacing Plotly’s default image button with a dropdown-based SVG/PNG export menu, persisting export events to localStorage, and adding an in-pane export history view with responsive layout adjustments. Sequence diagram for custom SVG/PNG export and history loggingsequenceDiagram
actor User
participant Pane
participant PlotPane
participant Plotly
participant LocalStorage
User->>Pane: click saveButton
Pane->>Pane: toggle exportMenuOpen
User->>Pane: select Export SVG or Export PNG
Pane->>PlotPane: handleDownload(format)
PlotPane->>Plotly: downloadImage(plotlyRef, format, filename)
PlotPane->>LocalStorage: getItem(visdom_export_log)
LocalStorage-->>PlotPane: existingLogJson
PlotPane->>PlotPane: saveExportLog(entry)
PlotPane->>LocalStorage: setItem(visdom_export_log, updatedLogJson)
PlotPane->>PlotPane: setExportHistory(loadExportHistory().reverse())
PlotPane->>Plotly: Plots.resize(plotlyRef)
PlotPane-->>User: image downloaded
User->>Pane: select Export history
Pane->>PlotPane: onShowExportHistory()
PlotPane->>PlotPane: toggle showExportHistory
PlotPane->>PlotPane: useEffect(showExportHistory)
PlotPane->>LocalStorage: getItem(visdom_export_log)
LocalStorage-->>PlotPane: historyLogJson
PlotPane->>PlotPane: setExportHistory(loadExportHistory().reverse())
PlotPane->>Plotly: Plots.resize(plotlyRef)
PlotPane-->>User: export history panel rendered
Class diagram for updated Pane and PlotPane componentsclassDiagram
class PlotPane {
+boolean smoothWidgetActive
+number smoothvalue
+boolean showExportHistory
+Array exportHistory
+loadExportHistory()
+updateSmoothSlider(value)
+saveExportLog(entry)
+handleDownload(format)
}
class Pane {
+string id
+string title
+any content
+Array widgets
+Array barwidgets
+boolean enablePropertyList
+boolean propertyListShown
+boolean exportMenuOpen
+handleZoom()
+handleMouseMove()
+handleClose()
+handleDownload(format)
+handleShowExportHistory()
}
PlotPane --> Pane : uses
PlotPane ..> LocalStorage : persists_export_log
PlotPane ..> Plotly : triggers_download_and_resize
class LocalStorage {
+getItem(key)
+setItem(key, value)
}
class Plotly {
+downloadImage(target, options)
+Plots.resize(target)
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider removing the console.log in handleDownload (or guarding it behind a debug flag) to avoid noisy logging in normal usage.
- The Plotly.Plots.resize calls wrapped in setTimeout are duplicated (after newPlot and in the showExportHistory effect); factoring this into a small helper would reduce repetition and keep resize behavior consistent.
- saveExportLog reimplements the localStorage parse logic instead of using loadExportHistory; using the helper there would centralize error handling and keep the export log behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider removing the console.log in handleDownload (or guarding it behind a debug flag) to avoid noisy logging in normal usage.
- The Plotly.Plots.resize calls wrapped in setTimeout are duplicated (after newPlot and in the showExportHistory effect); factoring this into a small helper would reduce repetition and keep resize behavior consistent.
- saveExportLog reimplements the localStorage parse logic instead of using loadExportHistory; using the helper there would centralize error handling and keep the export log behavior consistent.
## Individual Comments
### Comment 1
<location path="js/panes/PlotPane.js" line_range="57-66" />
<code_context>
+ }
+ };
+
+ const handleDownload = (format = 'svg') => {
+ console.log('handleDownload clicked', contentID, format);
+ const filename = `${contentID}_${new Date()
+ .toISOString()
+ .replace(/[:.]/g, '-')}`;
+
Plotly.downloadImage(plotlyRef.current, {
- format: 'svg',
- filename: contentID,
+ format,
+ filename,
});
+
+ saveExportLog({
+ contentID,
+ filename,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle `Plotly.downloadImage` success/failure instead of always logging a successful export
`saveExportLog` is always called with `status: 'success'`, but `Plotly.downloadImage` returns a promise and can fail. Chain `.then()` / `.catch()` on `Plotly.downloadImage`, log `status: 'success'` only on resolve, and `status: 'error'` (optionally with the error) on reject so export history reflects actual outcomes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+57
to
66
| const handleDownload = (format = 'svg') => { | ||
| console.log('handleDownload clicked', contentID, format); | ||
| const filename = `${contentID}_${new Date() | ||
| .toISOString() | ||
| .replace(/[:.]/g, '-')}`; | ||
|
|
||
| Plotly.downloadImage(plotlyRef.current, { | ||
| format: 'svg', | ||
| filename: contentID, | ||
| format, | ||
| filename, | ||
| }); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Handle Plotly.downloadImage success/failure instead of always logging a successful export
saveExportLog is always called with status: 'success', but Plotly.downloadImage returns a promise and can fail. Chain .then() / .catch() on Plotly.downloadImage, log status: 'success' only on resolve, and status: 'error' (optionally with the error) on reject so export history reflects actual outcomes.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
This PR improves the plot export workflow in Visdom by introducing a unified export menu and export history tracking.
Changes
Motivation
The current export functionality is limited and does not provide visibility into past exports. This improves usability and workflow for users working with multiple plots.
Testing
Screenshots
Summary by Sourcery
Introduce a unified export workflow for plots with selectable formats and an export history panel, while improving layout responsiveness when toggling history.
New Features:
Enhancements: