-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Export current line chart as SVG #1446
Conversation
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.
Awesome feature!
this.root = rootEl; | ||
} | ||
|
||
public asString(): string { |
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.
Nit, but asString()
seems like not quite the right name because to me it communicates turning the exporter itself into a string, which isn't what happens here. Something like exportAsString()
or exportToString()
or createString()
would seem clearer.
@@ -75,6 +77,20 @@ | |||
on-tap="_resetDomain" | |||
title="Fit domain to data" | |||
></paper-icon-button> | |||
<paper-menu-button on-paper-dropdown-open="_updateDownloadLink"> |
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.
Can you include a screenshot of what this UI looks like?
Also, it seems like this ultimately should be merged with data download links for CSV/JSON. Right now those are run-specific, but we've had a request for a "download all runs" option in #1232 and I think it would make sense to change the data downloads to always include all selected runs. If the user wants to download just one run, they can use the run selector to update the selected runs and then do the download. This would require a small amount of backend changes as well as frontend but it shouldn't be anything too difficult, and then it's easy to just expose CSV/JSON/SVG download options as a format choice only.
We could do this as a temporary thing, but even then it's probably good if it respects the "Show Download Links" checkbox, and it might reduce UI churn to just have an SVG button next to CSV and JSON that ignores the download-run dropdown entirely for now (until we remove it).
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.
This would require a small amount of backend changes as well as frontend but it shouldn't be anything too difficult
Just wondering, what warrants change in the backend? I must be not aware of some detail :(
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.
Here is the screenshot. I decided to put it under menu since I have to update href on <a>
only on demand (cannot trigger download programmatically; it has to be an anchor tag that user clicks). I can put data download options in there.
RE hiding the button: technically, the option says "Show data download links" and SVG may/may not fall under "data" category. However, especially if CSV/JSON downloads respect run-selector on the left instead of yet another run selector, I think it make sense to roll them under the same dot-dot-dot menu. What do you think?
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.
I think what would make the most sense for long-term state is to have a single blue icon showing a download arrow, like the iron-icons file-download
one, which is always visible (no more "Show data download links" checkbox).
Clicking that icon would pop up a small menu or panel with file formats - CSV, JSON, and SVG - which would download the plot data in the respective format, taking the currently selected runs into account.
The intermediate step addresses the need for a genuine user click on the anchor tag. We could use the menu-expansion to also trigger synthetic generation of the CSV/JSON data URL payloads so that we can reflect the run selection without needing any special backend endpoint.
Converting over the CSV and JSON ones is some extra work that we don't want to block this PR on, so for now I think this is fine, but I'd maybe vote for using the file-download
icon from the start rather than the vertical-dots one, since I think that makes it the new feature more discoverable, and would avoid changing the icons later.
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.
Ah about file-download
: I'd agree if the icon-button had a chevron on the right indicating that it is a menu button instead of just a button but it is quite misleading for what looks like a regular button to show a pop over.
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.
I disagree that it's "quite misleading" - I think sometimes it's helpful to show a chevron when available to indicate that there are multiple choices, but I don't think users are "misled" if clicking the button triggers a prompt before the action is completed.
There are many examples of icon buttons that show pop overs in widely used UI contexts, like the ubiquitous share button. Or for another example, every single one of these Gmail compose icons triggers a dialog or panel:
Perhaps you could say it's less common for a download button to have a pop over, but I think it's reasonable in this case where the format to use isn't obvious. In terms of communicating to users, an icon that's obviously for downloading but requires one extra step seems easier to understand than a generic menu icon with no reason to think downloading functionality would be there (especially as it already exists elsewhere in the UI today).
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.
There are many examples of icon buttons that show pop overs in widely used UI contexts, like the ubiquitous share button. Or for another example, every single one of these Gmail compose icons triggers a dialog or panel:
Point taken. Changed the icon!
Motivation: TB should give ability to save line chart as high quality SVG. Right solution: Modify Plottable to draw in one SVG with grouping instead of multiple SVG fragments. Problem: SVG creation is scattered and it may require a lot more effort to understand design choice around these fragments (may require dynamic creation of clipPath for masking a part of a figure) Current solution: Parse the Plottable drawn DOM and generate one SVG out of the DOM snapshot Shortcomings: - X-axis label may take up larger than required space - There are no labels to each line (should give an option when exporting)
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.
Ah, I had missed your last comment/update here, sorry for the delay.
With TB 1.12.0 on Firefox 63.0.1 on macOS, this yields an SVG that when opening, displays only this error:
There doesn't seem to be anything after |
It works in Safari, however. |
Motivation: TB should give ability to save line chart as high quality
SVG.
Right solution: Modify Plottable to draw in one SVG with grouping
instead of multiple SVG fragments.
Problem: SVG creation is scattered and it may require a lot more effort
to understand design choice around these fragments (may require dynamic
creation of clipPath for masking a part of a figure)
Current solution: Parse the Plottable drawn DOM and generate one SVG out
of the DOM snapshot
Shortcomings:
exporting)
This may require few iteration to constitute as done.