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

Fixed URL query update when zooming out of selected, labelled subtree #1151

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ktun95
Copy link

@ktun95 ktun95 commented Jun 2, 2020

Description of proposed changes

Fixes issue where zooming out of a selected labelled subtree does not corrected update the URL query.

Related issue(s)

Fixes #1138

Testing

I'm not entirely sure what sort of tests are appropriate for this change. Any feedback on this would be appreciated!

Thank you for contributing to Nextstrain!

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Thanks @ktun95!

This PR makes it so that if we are zooming out by clicking on the basal branch then the URL clade gets removed (cladeSelected=""). This works well for the test dataset from #1138, but is problematic when the clade we're zooming out to has it's own clade label (in which case we want to use it!).

I've made a newer test dataset with more capabilities - json here and nextstrain URL here. If you zoom to the "Laurasiatheria" lineage, then zoom out by clicking on the basal branch, we arrive at the "Boreoeutheria" lineage and we wish this to be displayed in the URL.


I hadn't read this code (the existing code, not your PR 😉 ) for quite some time and found it overly hard to comprehend. I refactored it to

  • make it much clearer that most of the code is actually getting the new branch label for the URL query by separating this into a function getBranchLabelForURLQuery
  • shorten onBranchClick and make it much easier to read
  • Access the object representing the branch we are actually zooming to, and use this to work out the appropriate branch label. This requires quite a bit of knowledge of the structure of the tree as we don't have many helper functions here.

This is commit 2eb3a50 (on top of this PR) which would be a suitable fix to the issues in itself, or you can use it (or a variant thereof) to modify this PR!

@ktun95
Copy link
Author

ktun95 commented Jun 8, 2020

@jameshadfield

Thanks for the feedback! I'll work on a more comprehensive solution. I would love to work off of your commit, but I can't figure out how to incorporate it into my local repository. Would it be poor practice to just copy and paste the changes into my local files in this case?

My apologies for the novice questions. I would really like to be able to contribute more to this project!

@jameshadfield
Copy link
Member

Would it be poor practice to just copy and paste the changes into my local files in this case?

Git can be tricky -- you're most welcome to copy my code bit-by-bit :)

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.

Zooming out from a labelled subtree does not correctly update the URL query
2 participants