Skip to content
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

Allow plugin to define type of downloaded file #71

Merged
merged 1 commit into from
Sep 21, 2020

Conversation

DanSava
Copy link
Contributor

@DanSava DanSava commented Sep 9, 2020

Closes #70

@DanSava
Copy link
Contributor Author

DanSava commented Sep 9, 2020

I could not find a suggestion on how the zip download is being tested so this PR does not contain any testing for the new functionality. I only did manual testing 😞

If this is approved and merged I will follow it up with a related issue and PR in webviz-config.

@anders-kiaer
Copy link
Collaborator

Thanks for the PR @DanSava 🎉

I could not find a suggestion on how the zip download is being tested so this PR does not contain any testing for the new functionality. I only did manual testing.

That is totally fine - we do not have an 🤖 integration test for the (zip) download button as of now.

If this is approved and merged I will follow it up with a related issue and PR in webviz-config.

We will get something like this in, but I'm thinking instead of adding a new special case (.csv) in addition to the existing special case (.zip), maybe we could simply remove the references to csv/zip altogether in webviz-core-components, and let plugin authors define what file type is to be downloaded? That also keeps the code base a bit smaller. Generally, in the future, I guess it could be basically anything that is downloaded from a plugin (SEGY cube behind some seismic intersection, some other binary domain specific file...).

Adding the possibility of choosing filename from plugin side is a nice extra change, good idea 👍 As well as changing download button icon to the more generic faDawnload.

Breaking changes in webviz-core-components can most probably be transformed into deprecation warnings in the only package (webviz-config) directly using webviz-core-components, so that is no problem in this repository.

Opinions on this @DanSava and @HansKallekleiv?

@DanSava
Copy link
Contributor Author

DanSava commented Sep 10, 2020

I'm thinking instead of adding a new special case (.csv) in addition to the existing special case (.zip), maybe we could simply remove the references to csv/zip altogether in webviz-core-components, and let plugin authors define what file type is to be downloaded?

I think this is a good idea

One small issue I can think about is that once there is only a general way of downloading a plot resource like csv or zip it will not be possible to implement both like have a separate button for zip and a another button for csv. But it is hard for me to estimate if this is something that users would want to have even in our application, so maybe it is not an issue at all.

@HansKallekleiv
Copy link
Collaborator

I agree. Handling this from the plugin side is better 👍

@anders-kiaer anders-kiaer added this to In Review in Webviz Sep 12, 2020
@anders-kiaer anders-kiaer changed the title Allow plugin to download csv file Allow plugin to define type of downloaded file Sep 13, 2020
Copy link
Collaborator

@anders-kiaer anders-kiaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀 Some minor comments to consider below, otherwise I think this is ready. 👍

Maybe most efficient to keep this PR open/unmerged until a corresponding webviz-config PR is made, in case it becomes obvious that some adjustments in this PR would be useful.

src/lib/components/WebvizPluginPlaceholder.react.js Outdated Show resolved Hide resolved
src/lib/components/WebvizPluginPlaceholder.react.js Outdated Show resolved Hide resolved
src/lib/components/WebvizPluginPlaceholder.react.js Outdated Show resolved Hide resolved
@anders-kiaer
Copy link
Collaborator

anders-kiaer commented Sep 20, 2020

Thanks for the changes both here and in the related webviz-config PR @DanSava - looks nice. Done some small further changes in separate commit (for now) - shout out if you don't agree @DanSava. 📢

@HansKallekleiv: can you do a final review on this one? ✔️ Then we'll merge and release a new version of webviz-core-components such that the version in webviz-config's setup.py can be properly bumped, as well as webviz-config CI becomes 🟢.

Copy link
Contributor Author

@DanSava DanSava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me 👍 thanks for making them!

@HansKallekleiv
Copy link
Collaborator

Looks good! 👍

Webviz automation moved this from In Review to Ready for merge Sep 21, 2020
@anders-kiaer anders-kiaer merged commit eb678d3 into equinor:master Sep 21, 2020
Webviz automation moved this from Ready for merge to Done Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Webviz
  
Done 🏁
Development

Successfully merging this pull request may close these issues.

Allow raw plot data to be downloaded directly to CSV
3 participants