tree: Add toggle to focus on selected #1373

trvrb · 2021-07-11T22:43:27Z

Description of proposed changes

This PR adds a toggle in the sidebar to emphasize visible nodes by expanding them to take up 80% of the vertical span of the panel.

focus-demo.mov

Related issue(s)

#1368

Design decisions

Add URL query? not in this PR but these would be the changes to make: 353b1e6
Button placement: within panel or in controls sidebar? Sidebar. Open to feedback on exact placement.
Tightly couple focus with zoom? We can discuss further in Improve visibility of filtered samples under various conditions #1368

Known bugs to fix

Other tasks

Detailed changelog description!
Merge base PR Tree component refactoring #1863
Post-merge: consider refactoring function definitions (ref)
Post-merge: consider renaming variables that use "display order" terminology (ref)

Original PR

Commit: e4b02c5

This is mostly working at this point, though there are a few issues (like initial render with ?z=zoom in URL does not work as it should due to node.visibility not existing in the initial calls to layout). I'm hoping first for user interface feedback (hence the draft PR).

Primary use case is to:

Filter to a trait value of interest (whether monophyletic or not)
Click "zoom to selected" to emphasize nodes with trait.

The biggest wins are being able to zoom to basal tips in the tree. For example, you can zoom to early SARS-CoV-2 samples or to clade 19A where this wasn't previously possible.

huddlej · 2021-07-12T17:13:36Z

I haven't had a chance to do a full review yet, but at first glance, this is a really cool feature! It seems pretty helpful for situations like H3N2 where we have recurrent mutations like 186D and we want to focus on these.

emmahodcroft · 2021-07-13T16:53:09Z

I haven't tested this out super-extensively yet, but I tried out zooming to 19B (currently can be quite difficult!) and also picking out a few sequences at the 'edges' of clades (which can also be extremely difficult to get a good look at, if they don't cluster but branch sequentially away from the clade). Both worked beautifully and gave super useful views that I don't think you can get with current auspice!

joverlee521 · 2021-07-14T00:38:51Z

It's really cool that the zoom works well with the date filter! 🤩

I noticed that the interaction between zooming via branches and the zoom button can be a little clunky.

Zoom in via a branch:
Then clicking on the zoom button brings up previously hidden branches on the bottom, making the x-axis hard to read:
Zooming back out via clicking on base branches leaves the tree in a strange configuration:

jameshadfield · 2021-07-16T04:46:39Z

The general behaviour is fantastic. I’m really happy to see the y-value-zoom functionality resurrected in this (much improved) form! Here’s a longer review of the functionality (I haven’t dug into the code at all yet, but I will).

Zoom to selected

My main issue is the confusing UI due to us mixing the y-zoom behaviour with the “zoom to selected button”.

(Overview of how this button currently works: it zooms into the MRCA of the selected tips - using the terminology of Auspice we change the in-view root node to be the MRCA of the tips which are both visible and in-view. If this node is the same as the current in-view root node, then the button isn’t selectable. Note that this MRCA node may not be visible, and thus you can’t always achieve this behaviour by clicking on the branch to zoom.)

Importantly, “zoom to selected” isn’t a flag or a state, it’s more like an action which is conditional on other state; this is why it’s so hard to store it as a URL query (see #1321).

With this PR, it’s really easy to get into intermediate states. For instance filter to Oceania, then zoom-to-selected, then inactivate the filter. The y-zoom is still focused on Oceania, but all the tips are visible again. There are plenty of other ones too (e.g. Jover’s screenshots), and it’s not clear when you’re in one of these intermediate states.

An alternative UI approach would be a “Focused zoom” toggle. With this on, any change in visibility (zooming, filtering etc) would trigger a recalculation of y values. This would look really nice when changing filters or animating (if it could be made fast enough). I think this UI would avoid all the intermediate states currently available in this PR, and make it extremely clear what mode we are currently in. It’s somewhat analogous to separating out the horizontal and vertical zooming.

Y value padding

In certain situations, (e.g. South America + 614D) the y-value calculations aren’t right. It looks like we’re setting some padding above selected tips, but not below them, which means the squashed branches are directly behind selected tips.

Do we still need the (existing) “zoom-to-selected” behaviour?

Perhaps not. I haven’t fully thought through all the use cases here, but I think we could remove this. We can still zoom into clades by clicking on branches to explore certain parts of the tree, which is essentially what the button does.

Zooming into early samples

The biggest wins are being able to zoom to basal tips in the tree.

This is much improved in this PR, but there are still some cases it doesn’t solve, as the upper bound of the time domain is always the latest date of the in-view tips. For instance, zooming into clade 1B of flu/seasonal/vic/ha/12y could be better:

(Perfect is the enemy of good, and I don’t think we need to address this in this PR.)

victorlin · 2024-09-13T18:32:26Z

Not able to build a new review app due to old NPM version. Rebase seems mostly trivial so I'm going to do that and hopefully get a working review app. Original commit is e4b02c5 if we want to revert back.

EDIT: I believe more work needs to be done to account for ca1046b and 00add58
EDIT: done

victorlin · 2024-09-16T21:09:46Z

PR is working now. I've updated the description with a link to a nextstrain.org preview instance and some remaining tasks. Also added some broader thoughts in the linked issue: #1368 (comment)

jameshadfield · 2024-10-06T21:19:38Z

I'm really excited about this functionality, as well as the next steps of x-zoom! Here's some comments after using the UI for ~30min, but I haven't looked at the code.

URL query
Let's disable the URL query for the first implementation. We've generally followed a pattern of testing a feature out for a while before adding the query - especially so for new functionality like this. Stepping back a bit, this relates to my comment above and also referenced in this related issue regarding a future where we drop the "zoom to selected" and just have focus. Or maybe vice-versa!

Y-value calculations
The y-value calculations are still not quite right - I think this is unchanged from my comment above. Using a filter with few samples makes this very obvious. Here's a screenshot from a subtree of Oropouche filtered to n=2 Columbia samples:

Focus on selected button/toggle UI:

The element looks to be clickable when a tree (with no filters) is first loaded, but it does nothing
How can we "disable" focus -- the element still appears to be clickable and says "focus on ..." and clicking it does nothing. Clicking "reset layout" restores the original view, but this wasn't obvious to me.
Is this supposed to act like a toggle or a button? I think we're aiming for the former¹. Assuming so, as we zoom the tree (clicking on branches / "zoom to selected" button) the y-values need to recalculate as different samples enter the viewport but they currently don't. Here's a screenshot of the above oropouche example where the focus was applied with n=2 samples selected and then I've zoomed out so now n=6 are selected but the new 4 are all squashed down the bottom (LHS) - the correct behaviour is on the RHS:

4. Assuming it is a toggle, the element should have a little on/off toggle or similar to convey this.

Inactivating / disabling a filter doesn't re-calculate the y-value (when "focused") -- this is the case I referred to in my original review about "intermediate states".

¹ Adding a "focus on selected" element by itself is clarifying, especially for a v1 design.

trvrb · 2024-10-07T22:22:20Z

Cool! Thanks so much for picking this up @victorlin. I have a couple comments (similar to James's above) based on UI testing.

I don't like adding things as options to the top right of a panel unless it's really something that should be an immediate click away (like "reset"). I'd suggest keeping this as a real toggle in the sidebar. It would act in a similar way as "Show confidence intervals" as changing the continuous behavior of the tree. I'd place this toggle at the end below "Explode Tree By" and I'd give it a similar experimental icon. For text I'd suggest "Focus tree on selected nodes".
Currently, the y-values aren't getting updated after new nodes are selected. To demonstrate go to https://nextstrain-s-nextstrain-l8gysc.herokuapp.com/zika?f_country=Colombia and then "Toggle focus" this updates y-values to focus on Colombia tips. However, then if you remove the Colombia filter and/or apply a USVI filter the y-values don't update. In "focus mode" the y-values should always reflect active tips.

I think that overall I recommend a "toggle" behavior and placing the toggle in the sidebar. I could see an argument for making it a button rather than a toggle ("focus on selected") that's essentially the current behavior. In this case I think it should really be possible to combine this button with the "zoom to selected" button. However, removing a filter with zoomed y-values results in a funky looking tree, like so:

while a toggle would prevent these funky trees from being accessible.

victorlin · 2024-10-07T23:50:23Z

@jameshadfield @trvrb thanks for the feedback! Switched to toggle behavior in fa421d5. PR description has been updated with a list of the clear bugs to tackle and open design decisions.

I've also done a bit of extra refactoring and will make a base PR with changes that can be considered before the rest of this PR is ironed out.

jameshadfield

Lots of comments - most minor, the only major one is around how display order is actually computed.

The functionality is generally working very well and will be a really nice improvement to Auspice.

.github/workflows/preview_on_downstream_repo.yaml

src/components/controls/toggle-focus.tsx

src/reducers/tree.js

src/components/tree/phyloTree/helpers.js

src/reducers/controls.ts

src/components/tree/phyloTree/helpers.js

jameshadfield · 2024-10-17T01:50:21Z

src/components/controls/miscInfoText.js

@@ -64,3 +64,11 @@ export const ExplodeTreeInfo = (
    It works best when the trait doesn&apos;t change value too frequently.
  </>
 );
+
+export const ToggleFocusInfo = (


[commenting on this line to enable a thread, it's not related to the info content]

Animation + focus gets a little crazy. I don't think it's worth resolving this at the moment - it's labelled "experimental" after all - but wanted to note it here.

Good note. Focus can be toggled on/off while animation is in progress, so I think fine to leave it as-is.

src/components/tree/phyloTree/helpers.js

This allows monitoring of filter changes in changePhyloTreeViaPropsComparison.

This will be used by the upcoming focus toggle.

jameshadfield

Just a few minor (<5min) changes left now, and also remember to drop the last 2 commits!

src/components/tree/reactD3Interface/change.js

jameshadfield · 2024-10-20T20:30:49Z

src/components/tree/reactD3Interface/change.js

+    args.newFocus = newProps.focus;
+  }
+
+  /* enable/disable focus */
+  if (oldProps.focus !== newProps.focus) {
+    args.newFocus = newProps.focus;
+    args.updateLayout = true;
+  }
+  /* re-focus on changes */
+  else if (oldProps.focus == true &&
+           newProps.focus == true &&
+           (zoomChange || dateRangeChange || filterChange)) {
+    args.newFocus = true;
+    args.updateLayout = true;


The naming of "newFocus" isn't quite right I think - sometimes it represents a change in focus state, but in the other 2 cases it simply represents the existing (unchanged) state as we need to pass this value to the setDisplayOrder function downstream. I'd suggest "focusState" or something.

(I know this is minor, but this section of code is relatively complex and when we re-read it months down the track little things like this become very helpful.)

+1! It seems similar to how scatterVariables is used. Renamed to simply focus and moved to another section in the function signature: 37d69ba

The naming tripped me up initially and this is definitely more clear.

src/components/tree/phyloTree/helpers.js

jameshadfield

Thanks for all the changes @victorlin - code looks good from my end, although the final three 🚧 commits need to be squashed with others etc. Parent PR #1373 also looks good. I'll let you merge both PRs at your convenience, then we can release a new version.

This commit adds a toggle in the sidebar to emphasize visible nodes by expanding them to take up 80% of the vertical span of the panel.

This function is a good candidate for timing: it recurses through all tree nodes and is called upon initial load and every update of filters or zoom level.

jameshadfield temporarily deployed to auspice-dynamic-yvalues-mqlvu0 July 11, 2021 22:43 Inactive

jameshadfield mentioned this pull request Jul 11, 2021

Test auspice PR 1373 nextstrain/nextstrain.org#376

Closed

trvrb temporarily deployed to auspice-dynamic-yvalues-jxzjzp July 22, 2021 00:08 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-n9rt9e September 13, 2024 18:22 Inactive

victorlin force-pushed the dynamic-yvalues branch from 82a9f76 to fe9d629 Compare September 13, 2024 18:35

nextstrain-bot temporarily deployed to auspice-dynamic-yvalues-76vqio September 13, 2024 18:35 Inactive

victorlin force-pushed the dynamic-yvalues branch from fe9d629 to 70e15e4 Compare September 13, 2024 18:39

nextstrain-bot temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 18:39 Inactive

victorlin added the preview on nextstrain.org label Sep 13, 2024

nextstrain-bot mentioned this pull request Sep 13, 2024

[bot] [DO NOT MERGE] Test Auspice PR 1373 nextstrain/nextstrain.org#1025

Closed

victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 21:03 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 21:16 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 21:27 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 22:05 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-ax3is5 September 13, 2024 22:27 Inactive

victorlin self-assigned this Sep 13, 2024

victorlin mentioned this pull request Sep 16, 2024

Improve visibility of filtered samples under various conditions #1368

Open

victorlin force-pushed the dynamic-yvalues branch from 7321e43 to 92b0560 Compare October 5, 2024 00:01

trvrb temporarily deployed to auspice-dynamic-yvalues-iqtpul October 7, 2024 22:06 Inactive

victorlin force-pushed the dynamic-yvalues branch from bc789eb to 5cdc564 Compare October 8, 2024 00:11

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 8, 2024 00:12 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 14, 2024 23:29 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 15, 2024 00:00 Inactive

victorlin force-pushed the dynamic-yvalues branch from ba86c18 to a6fdd6e Compare October 15, 2024 00:35

victorlin requested a review from jameshadfield October 15, 2024 00:35

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 15, 2024 00:35 Inactive

jameshadfield requested changes Oct 17, 2024

View reviewed changes

jameshadfield reviewed Oct 17, 2024

View reviewed changes

src/components/tree/phyloTree/helpers.js Outdated Show resolved Hide resolved

victorlin added 2 commits October 17, 2024 11:26

Add filters to tree props

0a1e821

This allows monitoring of filter changes in changePhyloTreeViaPropsComparison.

Add optional experimental icon to toggle component

cb65841

This will be used by the upcoming focus toggle.

victorlin force-pushed the dynamic-yvalues branch from a6fdd6e to 019ca4e Compare October 17, 2024 19:58

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 17, 2024 19:58 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 17, 2024 20:04 Inactive

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 18, 2024 22:43 Inactive

victorlin mentioned this pull request Oct 18, 2024

Downstream PR preview creation and build slowness/timeouts #1709

Closed

jameshadfield requested changes Oct 20, 2024

View reviewed changes

victorlin force-pushed the dynamic-yvalues branch from 1120581 to 126b895 Compare October 21, 2024 16:07

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 21, 2024 16:07 Inactive

victorlin mentioned this pull request Oct 21, 2024

ESLint: use type-safe equality operators #1872

Merged

3 tasks

victorlin requested a review from jameshadfield October 21, 2024 16:31

jameshadfield approved these changes Oct 21, 2024

View reviewed changes

Base automatically changed from victorlin/prep-dynamic-yvalues to master October 21, 2024 22:19

trvrb and others added 3 commits October 21, 2024 15:20

Add toggle to focus on selected

8a65633

This commit adds a toggle in the sidebar to emphasize visible nodes by expanding them to take up 80% of the vertical span of the panel.

Add timing functions to setDisplayOrder

a7b4057

This function is a good candidate for timing: it recurses through all tree nodes and is called upon initial load and every update of filters or zoom level.

Update changelog

b7f8bc9

victorlin force-pushed the dynamic-yvalues branch from 126b895 to b7f8bc9 Compare October 21, 2024 22:25

victorlin temporarily deployed to auspice-dynamic-yvalues-iqtpul October 21, 2024 22:25 Inactive

victorlin merged commit c370552 into master Oct 21, 2024
26 checks passed

victorlin deleted the dynamic-yvalues branch October 21, 2024 22:37

This was referenced Oct 21, 2024

Code refactoring #1874

Open

Adjust placement of focus toggle #1875

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tree: Add toggle to focus on selected #1373

tree: Add toggle to focus on selected #1373

trvrb commented Jul 11, 2021 •

edited by victorlin

Loading

huddlej commented Jul 12, 2021

emmahodcroft commented Jul 13, 2021

joverlee521 commented Jul 14, 2021

jameshadfield commented Jul 16, 2021

victorlin commented Sep 13, 2024 •

edited

Loading

victorlin commented Sep 16, 2024

jameshadfield commented Oct 6, 2024 •

edited

Loading

trvrb commented Oct 7, 2024

victorlin commented Oct 7, 2024

jameshadfield left a comment

jameshadfield Oct 17, 2024

victorlin Oct 17, 2024

jameshadfield left a comment

jameshadfield Oct 20, 2024

victorlin Oct 21, 2024

jameshadfield left a comment

tree: Add toggle to focus on selected #1373

tree: Add toggle to focus on selected #1373

Conversation

trvrb commented Jul 11, 2021 • edited by victorlin Loading

Description of proposed changes

Related issue(s)

Design decisions

Known bugs to fix

Other tasks

Original PR

huddlej commented Jul 12, 2021

emmahodcroft commented Jul 13, 2021

joverlee521 commented Jul 14, 2021

jameshadfield commented Jul 16, 2021

Zoom to selected

Y value padding

Do we still need the (existing) “zoom-to-selected” behaviour?

Zooming into early samples

victorlin commented Sep 13, 2024 • edited Loading

victorlin commented Sep 16, 2024

jameshadfield commented Oct 6, 2024 • edited Loading

trvrb commented Oct 7, 2024

victorlin commented Oct 7, 2024

jameshadfield left a comment

Choose a reason for hiding this comment

jameshadfield Oct 17, 2024

Choose a reason for hiding this comment

victorlin Oct 17, 2024

Choose a reason for hiding this comment

jameshadfield left a comment

Choose a reason for hiding this comment

jameshadfield Oct 20, 2024

Choose a reason for hiding this comment

victorlin Oct 21, 2024

Choose a reason for hiding this comment

jameshadfield left a comment

Choose a reason for hiding this comment

trvrb commented Jul 11, 2021 •

edited by victorlin

Loading

victorlin commented Sep 13, 2024 •

edited

Loading

jameshadfield commented Oct 6, 2024 •

edited

Loading