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

Unfurl clades during unrooted clade zoom #754

Merged
merged 2 commits into from
Jul 30, 2019
Merged

Unfurl clades during unrooted clade zoom #754

merged 2 commits into from
Jul 30, 2019

Conversation

trvrb
Copy link
Member

@trvrb trvrb commented Jul 29, 2019

This PR is specific for the unrooted aspect of PR #752.

This alters unrooted tree layout to give more radial territory to tips that are "inView" compared to tips that are out of view. This causes clade zoom to unfurl clades when zooming. Besides altering unrootedLayout, this commit adds an "updateLayout" flag that causes layouts to be recalculated on clade zoom to account for this. I believe this new behavior is now a definite improvement over previous behavior.

Previous, example Zika clade zoom:

zika-old

New zoom:

zika-new

There are separate improvements to make to margins, but I thought better to shelve them off from this PR.

This commit alters unrooted tree layout to give more radial territory to tips that are "inView" compared to tips that are out of view. This causes clade zoom to unfurl clades when zooming. Besides altering unrootedLayout, this commit adds an "updateLayout" flag that causes layouts to be recalculated on clade zoom to account for this.
@trvrb trvrb requested a review from jameshadfield July 29, 2019 22:25
and remove it from initial phylotree setup as it's only every used in unrooted layouts and each call to `unrootedLayout` will call it.
@jameshadfield
Copy link
Member

Looks awesome 🎉

(i) completely unrelated to this PR and too much work I've just realised that the tree is still rooted, in the sense that clicking on a node (e.g. zika top right) that's close to the root zooms into the rest of the tree, not that clade.

(ii) also beyond this PR we should add in a scale to the unrooted view!

(iii) also beyond this PR the clade label text should be made to not capture mouse events. You can't click on a branch behind text! (I probably didn't realise this as in rectangular trees the text isn't over branches)

(iv) code looks good. I removed addLeafCount from the initial phylotree setup as it's only ever used in unrooted layouts and each call to unrootedLayout will call it.

@trvrb
Copy link
Member Author

trvrb commented Jul 30, 2019

Thanks for the review James and for the code updates. This definitely made me want a scale in this view. And there might be a way to be smarter about text placement.

For (i), I don't immediately see how to avoid a "rooting" for these clade selections. If you point at a random branch we need to know whether the clade in question is to one side or the other of this branch. Without a rooting, I don't see how a single mouse click could specify right or left.

@jameshadfield
Copy link
Member

jameshadfield commented Jul 30, 2019

I don't immediately see how to avoid a "rooting" for these clade selections. If you point at a random branch we need to know whether the clade in question is to one side or the other of this branch. Without a rooting, I don't see how a single mouse click could specify right or left.

Yeah, and there's no way it's worth the effort it would take to sort out. Typically, in unrooted views I think you'd want to zoom into the side of the branch with fewer tips. But again, not a good investment of time for us.

@trvrb trvrb merged commit 841b249 into master Jul 30, 2019
@trvrb trvrb deleted the unrooted-zoom branch July 30, 2019 02:29
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.

2 participants