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 metadata export of selected tips #1055

Closed
jameshadfield opened this issue Apr 10, 2020 · 10 comments
Closed

Allow metadata export of selected tips #1055

jameshadfield opened this issue Apr 10, 2020 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@jameshadfield
Copy link
Member

jameshadfield commented Apr 10, 2020

Context
Currently we provide a way to export metadata of each strain (read: genome) in the dataset via the "download data" button in the footer. This always exports data for each and every genome in the dataset.

Description
There should be an additional entry which allows downloading of only those genomes which are selected. Here "selected" means included in a filter or a zoomed in part of the tree. Programmatically this is inView && visible.

Examples

image
https://nextstrain.org/ncov/global?branchLabel=clade&dmin=2020-03-04&f_country=Switzerland&label=clade:A2 would allow a metadata TSV describing only the 17 selected genomes.

I'll try to pick this up over the coming days unless there are takers!

@jameshadfield jameshadfield added the enhancement New feature or request label Apr 10, 2020
@frogsquire
Copy link
Contributor

Hi @jameshadfield,

I'd like to take this, if you'd be willing to give it to me. It seems like a good next step from the (small) work I did for #915.

I should have time to make substantial progress on this over the next week or so.

@jameshadfield
Copy link
Member Author

jameshadfield commented Apr 12, 2020

Would be great! Here's a quick mock of what I'd like to change the display of the download data modal to:

image

(You don't have to do this as part of this PR, but can if you want! If not, please don't spend too much time tweaking the rendering of the icons etc as they are currently displayed.)

frogsquire added a commit to frogsquire/auspice that referenced this issue Apr 12, 2020
…nodes when metadata generated via this method
@frogsquire
Copy link
Contributor

@jameshadfield One point to discuss - what should the filename for the downloaded .tsv be if only selected data is included?

An easy option would be e. g. nextstrain_dataset_selected_metadata.tsv. If we wanted to be a little fancier, we could try to include some of the filter criteria - but for very specific filters, that could result in quite long filenames (which potentially we'd then have to decide when to truncate).

Do you have a preference?

@jameshadfield
Copy link
Member Author

I think trying to enumerate the filtering in the filename isn't the way to go, and for certain tree zooming there's no "name" to even put in the filename. I'd just go with nextstrain_dataset_selected_metadata.tsv.

frogsquire added a commit to frogsquire/auspice that referenced this issue Apr 12, 2020
@frogsquire
Copy link
Contributor

frogsquire commented Apr 15, 2020

The basic functionality is working as of my last commit. I'm now working on updating the button text and adding those labels. @jameshadfield, did you happen to have CSS for your mocked version that you'd want to see used? If not, I will approximate it based on the image and what looks good at reasonable screen sizes.

A couple of final questions before I finish up and make a pull request:

  • When I set up my dev environment for auspice, npm updated package.json and package-lock.json after I ran npm install, and later npm update. Should I discard/not commit changes to the package configs?
  • We already have code for showing XXXX of YYYY datasets selected. Is there something similar for counting authors? (If not, I can create it from the nodes, of course.)
  • How do we handle internationalization updates for the new language? (In terms of letting people know there's new text to translate.)

If the latter two questions are too in the weeds, we could always split updating the menus to a new issue.

@jameshadfield
Copy link
Member Author

Great! Thanks @frogsquire. I don't have any CSS for the mock, up to you if you're happy playing around with it or would like to leave it for another PR.

Is there something similar for counting authors?

I don't think so, but as you say should be easy to loop over nodes (and quick, considering it's only done once). Happy to leave this out of the PR, I got carried away making the mock.

How do we handle internationalization updates for the new language?

We don't really have a process here. It will nicely fall back to english, which is a perfectly acceptable medium-term solution. Running npm run diff-lang will identify unused keys & untranslated keys, but we don't have a good way to solicit new translations for keys which get updated.

npm updated package.json and package-lock.json

It shouldn't have updated package.json, as far as my understanding of npm goes. package-lock.json does get updated a fair bit, but I'd try to leave this out unless package.json updated (e.g. due to a new dep). I tend to regenerate the package-lock.json from time to time, especially when we release new versions.

frogsquire added a commit to frogsquire/auspice that referenced this issue Apr 15, 2020
@frogsquire
Copy link
Contributor

I don't have any CSS for the mock, up to you if you're happy playing around with it or would like to leave it for another PR.

No problem. I think I can do it in this one, with the numbers as in the mock. Should have something up in the next day or so.

We don't really have a process here. It will nicely fall back to english, which is a perfectly acceptable medium-term solution. Running npm run diff-lang will identify unused keys & untranslated keys, but we don't have a good way to solicit new translations for keys which get updated.

OK, just thought I would check.

It shouldn't have updated package.json, as far as my understanding of npm goes. package-lock.json does get updated a fair bit, but I'd try to leave this out unless package.json updated (e.g. due to a new dep). I tend to regenerate the package-lock.json from time to time, especially when we release new versions.

I think it must be the npm update that did it? All the packages in my changed package.json are just new versions. You can see it here: frogsquire@b972a23

but I'd be happy to revert it so as not to clutter the pull request.

frogsquire added a commit to frogsquire/auspice that referenced this issue Apr 16, 2020
@frogsquire
Copy link
Contributor

@jameshadfield Pull request is up. Please let me know if you'd prefer the package changes be reverted/removed from the PR.

In the meantime, a few notes:

  • While the code to determine whether the node is part of selected data to be downloaded takes into account node.inView, the code to display the number of selected nodes doesn't - because it's the same code that's used at the top of the page above the tree, getNumSelectedTips. I added a todo in code - I'm not sure if this disparity is intentional or not.
  • I noticed that author objects which come out of getFullAuthorInfoFromNode have generally identical author and value properties. Is there a reason for this?
  • I did add the descriptions to the modal, and tried to style it to look as close to your mock as possible. It should also wrap acceptably at low resolutions.
  • When the modal is big enough, or the Chrome dev console is open, the modal can sometimes clips over or even under the side menu area, obscuring text because the colors are more similar (or the modal is behind it) - is this something to make an issue for? Can send screenshots.

frogsquire added a commit to frogsquire/auspice that referenced this issue Apr 17, 2020
frogsquire added a commit to frogsquire/auspice that referenced this issue Apr 17, 2020
frogsquire added a commit to frogsquire/auspice that referenced this issue Apr 17, 2020
@frogsquire
Copy link
Contributor

@jameshadfield Not sure why, but my PR isn't showing up as linked to this - perhaps my fault for not calling it "#1055" instead of "1055" initially?

Either way, is this resolved with that PR (nonwithstanding my questions above)?

@jameshadfield
Copy link
Member Author

Yes it is -- thanks for the fix 😄

The released version of auspice with PR #1067 (2.13.0) is now on dev.nextstrain.org and as long as there are no bugs will become part of nextstrain.org in ~24 hours

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

No branches or pull requests

2 participants