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

Zoom into animation has broken #1191

Closed
trvrb opened this issue Jul 25, 2020 · 1 comment · Fixed by #1192
Closed

Zoom into animation has broken #1191

trvrb opened this issue Jul 25, 2020 · 1 comment · Fixed by #1192
Labels
bug Something isn't working

Comments

@trvrb
Copy link
Member

trvrb commented Jul 25, 2020

Current Behavior
When zooming into a clade, the tree now snaps to the zoomed clade rather than doing a proper zoom animation. Interestingly, when "reset zoom" is run the zoom out animation occurs as intended. This bug in behavior can be ready seen in https://nextstrain.org/zika or https://nextstrain.org/groups/blab/sars-like-cov. I think it was missed in https://nextstrain.org/ncov/global, just because it's hard to see the animations in large trees due to lag.

Expected behavior
When zooming into a clade, there should be animation going from original view to zoomed view.
@trvrb trvrb added the bug Something isn't working label Jul 25, 2020
@jameshadfield
Copy link
Member

jameshadfield commented Jul 28, 2020 •
edited
Loading

Good bug! We keep track of when the previous tree manipulation happened, and if it was within the last second then we don't animate (for performance reasons). This bug appears to be due to a branch-hover event being incorrectly counted as such a manipulation (wasn't the case in v2.4). If you hover on a branch for over a second before clicking you'll see it zooms as desired. Should have a fix us shortly.

jameshadfield added a commit that referenced this issue Jul 28, 2020
@jameshadfield
Improve redux state recalc on narrative slide changes. Closes #1191
0ed3eee 
This commit improves the creation of new state during narrative slide-changes which has a number of beneficial side-effects.

Previously we made a shallow-copy of the (redux) controls state when we changed slides, which resulted in props-comparison of nested state (i.e. objects within the controls state) being hard to reason with. This was problematic with the `temporalConfidence` object, which explains the approach employed in 0713e69 to allow narrative slides to update CIs.

We now deep clone the controls state, allowing valid props comparison and allowing us to use this technique to decide when to render / remove CIs. (This is essentially a reversion of 0713e69, which was implemented as props comparisons were found to be unreliable).

This commit was motivated by, and fixes #1191. That bug was a result of the previous logic indicating that CIs should be removed on each branch-hover event. This meant that any subsequent tree-manipulation (such as clicking a branch to zoom) would be likely to be considered "in quick succession" to a previous manipulation and thus not employ an animation.

A test narrative is added.
@jameshadfield jameshadfield mentioned this issue Jul 28, 2020
Merged
jameshadfield added a commit that referenced this issue Aug 3, 2020
@jameshadfield
Merge pull request #1192 from nextstrain/fix-zoom
08e9522 
Improve redux state recalc on narrative slide changes. Closes #1191
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve redux state recalc on narrative slide changes. Closes #1191 nextstrain/auspice
2 participants
@trvrb @jameshadfield