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

Reinstate (improved) download ability #587

Merged
merged 18 commits into from
Jul 3, 2018
Merged

Reinstate (improved) download ability #587

merged 18 commits into from
Jul 3, 2018

Conversation

jameshadfield
Copy link
Member

@jameshadfield jameshadfield commented Jun 28, 2018

This PR re-enables downloading of data, and closes #562.

Improvements

  • The SVG is now saved as a single file, essentially mirroring the positioning of the panels as they appear in the browser.
  • Information text is written to the SVG
  • Pressing the magic key d is a shortcut for downloading the SVG
  • The Nextstrain paper citation has been added to the auspice footer.
  • Map panning is correctly rendered in the SVG (most of the time)

Known bugs

  • changing datasets updates the map in the DOM, but upon SVG download the old map is written. Unsure how to fix this (I've tried tearing down window.L and/or this.state.map.remove() within <Map>, however this causes fatal leaflet errors. I remember @colinmegill having troubles here so don't think the fix is simple.)
  • sometimes map panning causes SVG elements outside the map-panel to be written to the SVG. I tried applying a clipPath, however this resulted in internal elements which crossed the boundary also being removed.

In general, the mapDemesAndTransmissions SVG is rather complicated and @colinmegill would have the most knowledge here. On the flipside, when the map contains errors they are rather obvious ;)

@jameshadfield jameshadfield requested a review from trvrb June 28, 2018 21:35
@trvrb
Copy link
Member

trvrb commented Jun 30, 2018

@jameshadfield --- This is great. Some small things that I noticed in review:

  1. The download modal should be larger given all the text that's squeezed into it. This is especially apparent when window is small:

download_modal

I wonder about instead making it take up the entire display and give it a big x in the top right?

This is also because it's not visually consistent with the other modals. Other modals are dark grey background with rounded corners and white text. If you're going for a modal where you click outside, make it consistent with other modals in the app.

  1. Newick should be downloaded as .nwk to match augur format.

  2. I'd much prefer metadata as .tsv to .csv. This is my general preference across the board. Was csv an active decision?

Copy link
Member

@trvrb trvrb left a comment

Choose a reason for hiding this comment

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

@jameshadfield
Copy link
Member Author

@trvrb points 1-3 have been addressed now and I think this is good to go.

@jameshadfield
Copy link
Member Author

This PR closes #450 and closes #429

Copy link
Member

@trvrb trvrb left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you for humoring me here James.

@trvrb
Copy link
Member

trvrb commented Jul 3, 2018

One small comment that's worth thinking about even if I don't have an immediate solution: The download modal states

Showing 6 of 502 genomes, from 1 author, from 1 region, from 5 countries, dated Jul 2011 to Jan 2018.

But then clicking download "strain metadata" or "author metadata" downloads all 502 strains and 82 authors.

@trvrb
Copy link
Member

trvrb commented Jul 3, 2018

Go ahead and merge whenever you'd like @jameshadfield.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reinstate & improve download data button / page
2 participants