-
Notifications
You must be signed in to change notification settings - Fork 168
Focal zoom mode acts on both axis #1993
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
Conversation
|
I moved this forward a little bit today, mainly by allowing the "focus on selected" toggle to now toggle between the "old" auspice layout and the updated focus layout which essentially defines the viewport as the smallest rectangle which includes all branches/tips which are "on" as well as using the accordian-zoom. There's still some little bugs, and probably more undiscovered ones. The biggest change is that the |
540c43e to
b3d97cf
Compare
src/components/tree/tree.tsx
Outdated
| export default withTranslation()(TreeComponent); | ||
|
|
||
|
|
||
| function TreeButtons(props: TreeComponentProps): ReactElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is subtlety around how the tree buttons ("zoom to selected" etc) behave with this functionality!
Situation 1 ZIKV, filtered to USVI & focus on Selected:
NOTE: the green "root" shown in focus on selected is not tracked in redux state - it only looks like a root due to a viewport shift. Let's call this the "viewport root". We can figure it out with a tree traversal to find the first visible node.
What does clicking on the viewport root and each of the buttons do in this context? My thoughts:
- "-" doesn't make sense and should be inactivated. (Same as clicking on the viewport root branch)
- "zoom to selected" doesn't make sense, as we're continually keeping things focused on selected!
- "zoom to root" - should be shown inactivated (see situation 3)
Situation 2
Things get more interesting when we shift the min date slider, as we are presented with a situation Auspice has so far completely avoided:
We now have three viewport roots. In this context:
- clicking on any of the viewport roots zooms into that subtree (current behaviour)
- "-" doesn't make sense and should be inactivated
- "zoom to selected" doesn't make sense and should be hidden
- "zoom to root" - should be shown inactivated (see situation 3)
Situation 3 when zoomed into a USVI subtree, e.g:
- clicking on the viewport root does make sense and should zoom out (taking us towards the screenshots in situation 1 or 2, depending on the min-date slider value)
- "-" should do the same
- "zoom to selected" doesn't make sense and should be hidden
- "zoom to root" does make sense, and we should zoom out to the screenshots in situation 1 or 2, depending on the min-date slider value. This button should be renamed to something like "show all selected" or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @trvrb does this line up with your expectations?
Simplifies the main `TreeComponent` which is a welcome change in and of itself, and also sets the stage for upcoming changes such as <#1993> which will add complexity to the buttons functionality, as well as the next commit...
b3d97cf to
a29245f
Compare
|
Thanks for showing that this is possible! My main concern is the conflation of "zoom" and "focus" as defined in #1368 (comment). I'd suggest putting this new feature behind its own toggle, at least for testing purposes. Example: 82c3e35 It might make more sense to use this feature to replace the existing "Zoom to selected" button, rather than adding it to the "Focus on selected" toggle. |
| } | ||
|
|
||
|
|
||
| function setFocus(newToggleValue: null|"selected"): ThunkFunction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[@victorlin] My main concern is the conflation of "zoom" and "focus" as defined in #1368 (comment).
For testing purposes, perhaps a review app of 82c3e35 would be good enough? Conceptually this PR can split them out, but there would be a lot of lines of changed code.
There's always a tension between more controls & complexity vs simplicity but less control. The conversations I had with Trevor led us to the view that exposing a simpler UI here was preferred. I'd like to hear what others think about this...
P.S. Your definitions of "zoom" vs "focus" are really helpful for internal discussions, but I don't think there's a problem calling everything "focus" in the Auspice UI if we do choose to use a single toggle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR for review app: #2028
Allowing views as described in #1993 (comment) would be the only reason to distinguish "zoom" vs. "focus" in the UI. I think it's fine to combine under a single toggle if there's not much benefit to having such views – I'm not a user so I can't tell!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After playing around with both, I'd prefer the simpler UI with just a single toggle.
| <button style={activeZoomButton ? selectedButtonStyles : unselectedButtonStyles} onClick={zoomToSelected}> | ||
| {props.t("Zoom to Selected")} | ||
| </button> | ||
| {showZoomButton && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[@victorlin] It might make more sense to use this feature to replace the existing "Zoom to selected" button, rather than adding it to the "Focus on selected" toggle.
I think it's a toggle state not a button action, as I like the dynamic behaviour as you change the date slider / filters. But you're right that they are similar, which is why I got rid of the "zoom to selected" button when in focus mode!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[@victorlin] This enables views such as "zoom only" below which are currently not possible in this PR.
I might be confusing things here, but seems like the "zoom only" view is possible in this PR if you switch off the "Focus on selected" toggle in the sidebar. It took me a bit to realize this was possible since I had to scroll down the sidebar to see the toggle was on.
My feedback here would be to make the "Focus on Selected" toggle more prominent and/or some other indication that the tree is currently in the "focused" state. Since it's expected to be used in conjunction with the date slider/filters, I'd move it to closer to those controls. Maybe the top of the "Tree" section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be confusing things here, but seems like the "zoom only" view is possible in this PR if you switch off the "Focus on selected" toggle in the sidebar.
I don't think so – switching off "Focus on selected" disables both zoom and focus. Here's the comparison using review app from #2028:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, thanks for the examples in the review app. I was mixing up your idea of "dynamic zoom" with the existing "zoom to selected" button.
|
I was just reaching for this to try to look at https://nextstrain.org/measles/genome?c=division&dmin=2025-01-01&f_country=USA&focus=selected. It would be nice to have this merged (I didn't actually realize it wasn't). I do think that a single toggle to "Focus on selected" that applies both to y values and to x values makes sense semantically to me. Note with this measles example I can't click "Zoom to selected" in the tree panel. |
Fixes a (never encountered?) bug where both can be enabled via URL queries, which led to an inconsistent / broken UI. (This state wasn't possible to get into by interacting with Auspice.)
Fixes an omission where the `changeDateFilter` action wouldn't set certain tree properties on the action and the reducer would therefore set them to undefined. This became apparent in (forthcoming) temporal zoom work.
This simplifies calling functions and makes the forthcoming work on
temporal focus much more straightforward.
While this follows an established pattern in Auspice it makes
out-of-sync errors easy. We should explore allowing PhyloTree to simply
see into the redux store's controls & {tree,treeToo} state and then the
`change` functions can focus (!) on working out which rendering
functions to call and forget about having to also update phylotree state.
This causes date updates and other filters to dynamically update the XY bounds, ensuring that the tree fills the page. This works well for zooming to early samples and give a generally more dynamic feeling. It combines very well with "focus on selected", which is why we combine the two features. Co-authored-by: james hadfield <jameshadfield@users.noreply.github.com> Co-authored-by: Trevor Bedford <trevor@bedford.io>
When we're focused there is no state where this button makes sense, as focus means we change the viewport to surround the selected nodes (where selected is the intersection of filters, date range and selected subtree (inViewRootNode))
Simplifies the main `TreeComponent` which is a welcome change in and of itself, and also sets the stage for upcoming changes such as <#1993> which will add complexity to the buttons functionality, as well as the next commit...
a29245f to
1ff6c66
Compare
37f0352 to
c053fc8
Compare

This causes date updates and other filters to dynamically update the XY bounds, ensuring that the tree fills the page. This works well for zooming to early samples and give a generally more dynamic feeling. It combines very well with "focus on selected".
I believe that this could hacked on to combine with "focus on selected", but the intent here is just to produce a review app for playing around with.