All the zooms #886
Merged
All the zooms #886
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameshadfield
temporarily deployed
to
auspice-all-the-zooms-ygdha54l
jameshadfield
force-pushed
the
all-the-zooms
branch
from
d28aa2a to
835a6cd
Compare
jameshadfield
temporarily deployed
to
auspice-all-the-zooms-ygdha54l
jameshadfield
temporarily deployed
to
auspice-all-the-zooms-ygdha54l
|
|
A bug in `createStateFromQueryOrJSONs` caused the color scale to be recreated unnecessarily on each page change in a narrative. This was somewhat harmless, but meant that the map logic was confusing as it would tend to remove D3 elements & recreate them unnecessarily (as it detected a color-by change), which was different to the expected behavior outside of narratives.
`maybeDrawDemes...` doesn't need to be called - all we need to do is change the map bounds without recomputing / redrawing any d3 elements
The previous implementation did not work on a lot of datasets (`demeIndices` and `demeData` seem to cause a lot of issues). This commit breaks out the logic into a method `moveMapAccordingToData` which can be called from various places according to the react updates taking place. This is also the only place where the map is moved except for (a) the RESET ZOOM button and (b) when the user pans / zooms. Effects: * Loading a dataset with filters enabled zooms to the filtered data * Narratives: changing filters between pages causes map zoom * Narratives: changing geo res between pages causes map zoom Also adds a number of console logs which should be cleaned up before release.
Already in narrative mode, if filters are enabled or geoRes changes, the map updates to fit the data in the screen. They weren't implemented for normal view, as moving the map can be annoying when you are interacting with the data. Here we keep track of if a user has panned / zoomed the map at all (and this is reset upon pressing the "RESET ZOOM" button). If they are yet to interact with the map, then we do manipulate it when the filters (etc) change. Note that clicking on the animation button "counts" as a user interaction. This avoids us constantly resizing the map during animation.
We need to run the `updateOnMoveEnd` function when the map loads, in order to correctly translate the d3elements. Due to a race condition, this wasn't being run for ebola -- potentially because the dataset wasn't drawn until after the map was ready?
If a user is yet to interact w. the map, then it will automagically zoom according to geo-res changes, filter changes etc. Clicking the "reset zoom" button now also resets the "user has interacted with map?" state (which was the original intention).
This shifts the code usage to mirror the intended URL query. By removing all old references to `query.clade` we reduce potential confusion. By keeping a string type for `<key>:<value>` we simplify the code and keep it in sync with the mental model for how the URL query looks. These changes also allow zooming to root in narratives when no label (clade) is specified in the URL.
Expected behavior: (1) By default, "clade" will be shown, if present in the tree. (2) If a JSON defines `branch_label` in the `display_defaults`, then this becomes the default shown in auspice. (3) if a URL query `branchLabel` is set, this changes the current view.
a bug was found where the docstring described the original intention, but the code was doing something else. Updated the calling functions & docstrings to match what the code does. No change in behavior.
Errors were happening because the function was not being given the `dispatch` function. Dispatch is used to trigger notification banners (in this case). In future we should consider `createStateFromQueryOrJSONs` returning any errors / warnings to be handled higher up.
This was infrequently used in the past to trigger a zoom-to-root in the tree. This behavior now occurs if you simply do not provide a clade / label query key. To avoid unnecessary warning notifications we deal with this here.
Narratives which were zoomed into a part of the tree were liable to lose this zoom when not showing then reshowing a tree -- this is because the zoom setting wasn't always being applied to the tree when it was set up.
jameshadfield
force-pushed
the
all-the-zooms
branch
from
a7755f8 to
8397e40
Compare
jameshadfield
temporarily deployed
to
auspice-all-the-zooms-ygdha54l
|
Merging this after a lot of testing by @emmahodcroft and myself. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR representing #870 + #861 + bugfixes. See previous PRs for discussion.