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.

Sign up for GitHub

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

Jump to bottom

Tree render order #612

Merged
merged 6 commits into from
Aug 5, 2018
Merged

Tree render order #612

merged 6 commits into from
Aug 5, 2018

Conversation

jameshadfield
Copy link
Member

This PR is in response to #610 (comment), which it fixes.

The main change is that tree elements are now collected into named groups, and these groups persist so that later renders (e.g. upon update) preserve the correct layering / ordering.
As a bonus, it's now much clearer to debug the elements as they are collected & named:

image

Additionally

  • Vaccine dotted lines are removed as they were zero-length but being drawn in the DOM
  • T-branch elements are not drawn into the DOM for clock/unrooted layouts

@jameshadfield jameshadfield mentioned this pull request Aug 4, 2018
Merged
@trvrb
Copy link
Member

trvrb commented Aug 4, 2018

@jameshadfield ---

I really like collecting groups here. Makes a more inspectable SVG and will make an SVG that is easier to edit in Illustrator etc...

Two issues:

  • There are two id="majorGrid" group elements. If two should exist, they should have different ids. Ah. One should be minorGrid.

  • I was able to break confidence bands by just mousing around the tree resulting in confidence bands that refuse to show up. I couldn't reproduce this on live.

@jameshadfield
Store & reuse <g> elements for the tree grid
0d2af5f 
By storing references to <g> elements, we don't have to create new ones upon updates. This also preserves the initial ordering of groups, avoiding bugs such as #610 (comment)
@jameshadfield
Collect remaining tree SVG elements into named groups
d48f21b 
See previous commit for "why"
@jameshadfield
remove (unused) dotted vaccine line from DOM
14f7d25
@jameshadfield
Don't draw branch-T elements for unrooted / clock tree layouts
a706d9a 
Reduces the number of elements in the DOM
@jameshadfield
Copy link
Member Author

Yup, one majorGrid should have been minorGrid. That's fixed now.

@jameshadfield
immediately remove CIs onBranchLeave to prevent d3 transition bugs
71d4644 
Problem: D3 selection, with transitions, seems to be lazily evaluated such at it would grab the newly displayed CI as well and remove it. Evaluating .selection() prior to setTimeout didn't fix this. This wasn't a problem before as we just added CIs in different DOM groups.

Solution: instantaneously remove the CIs on branch leave.
@jameshadfield
Copy link
Member Author

I was able to break confidence bands by just mousing around the tree resulting in confidence bands that refuse to show up. I couldn't reproduce this on live.

Now fixed via 71d4644 👍

@trvrb
Copy link
Member

trvrb commented Aug 5, 2018

This is great @jameshadfield. Everything working as far as I can tell. Please go ahead an merge.

@jameshadfield jameshadfield merged commit 7176134 into master Aug 5, 2018
@jameshadfield jameshadfield deleted the tree-render-order branch August 5, 2018 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trvrb trvrb trvrb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jameshadfield @trvrb