From bde3d870f63c9174e1b891f7efbbcf1efa6367f3 Mon Sep 17 00:00:00 2001 From: Emma Hodcroft Date: Thu, 30 Jan 2020 18:26:53 +0100 Subject: [PATCH 01/22] merge master and zoom_button_with_filter --- src/components/map/map.js | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index a86706800..814eda444 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -207,11 +207,6 @@ class Map extends React.Component { if (mapIsDrawn && allDataPresent && demesTransmissionsNotComputed) { timerStart("drawDemesAndTransmissions"); /* data structures to feed to d3 latLongs = { tips: [{}, {}], transmissions: [{}, {}] } */ - if (!this.state.boundsSet) { // we are doing the initial render -> set map to the range of the data - const SWNE = this.getGeoRange(); - // L. available because leaflet() was called in componentWillMount - this.state.map.fitBounds(L.latLngBounds(SWNE[0], SWNE[1])); - } const {demeData, transmissionData, demeIndices, transmissionIndices} = createDemeAndTransmissionData( this.props.nodes, @@ -239,6 +234,12 @@ class Map extends React.Component { this.props.pieChart ); + if (!this.state.boundsSet) { // we are doing the initial render -> set map to the range of the data + const SWNE = this.getGeoRange(demeIndices, demeData); + // L. available because leaflet() was called in componentWillMount + this.state.map.fitBounds(L.latLngBounds(SWNE[0], SWNE[1])); + } + /* Set up leaflet events */ // this.state.map.on("viewreset", this.respondToLeafletEvent.bind(this)); this.state.map.on("moveend", this.respondToLeafletEvent.bind(this)); @@ -304,16 +305,21 @@ class Map extends React.Component { this.setState({demeData: newDemes, transmissionData: newTransmissions}); } } - getGeoRange() { + // Allow to pass in particular demeIndices & demeData for initial render, when these aren't officially set yet + getGeoRange(demeIndices = this.state.demeIndices, demeData = this.state.demeData) { const latitudes = []; const longitudes = []; + // Check the count data - if it has a count of 0, it's not being drawn, so don't include in Geo range! this.props.metadata.geoResolutions.forEach((geoData) => { if (geoData.key === this.props.geoResolution) { const demeToLatLongs = geoData.demes; Object.keys(demeToLatLongs).forEach((deme) => { - latitudes.push(demeToLatLongs[deme].latitude); - longitudes.push(demeToLatLongs[deme].longitude); + if ((!demeIndices && !demeData) || // if we haven't loaded these yet, take all locations + (demeIndices && demeData[demeIndices[deme]].count !== 0)) { // if have, only those with counts! + latitudes.push(demeToLatLongs[deme].latitude); + longitudes.push(demeToLatLongs[deme].longitude); + } }); } }); From 8a213ac8976c34df1182a441336bc18cc48401e2 Mon Sep 17 00:00:00 2001 From: Emma Hodcroft Date: Thu, 30 Jan 2020 18:33:13 +0100 Subject: [PATCH 02/22] detect change in geoRegion via filter too --- src/components/map/map.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index 814eda444..d04442545 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -234,10 +234,16 @@ class Map extends React.Component { this.props.pieChart ); + const SWNE = this.getGeoRange(demeIndices, demeData); + const maybeNewBounds = L.latLngBounds(SWNE[0], SWNE[1]); if (!this.state.boundsSet) { // we are doing the initial render -> set map to the range of the data - const SWNE = this.getGeoRange(demeIndices, demeData); // L. available because leaflet() was called in componentWillMount - this.state.map.fitBounds(L.latLngBounds(SWNE[0], SWNE[1])); + this.state.currentBounds = maybeNewBounds; + this.state.map.fitBounds(maybeNewBounds); + } else if (!this.state.currentBounds.equals(maybeNewBounds)) { + // check to see if the new bounds would be different for any reason - if so, change them! + this.state.currentBounds = maybeNewBounds; + this.state.map.fitBounds(maybeNewBounds); } /* Set up leaflet events */ @@ -551,6 +557,7 @@ class Map extends React.Component { fitMapBoundsToData() { const SWNE = this.getGeoRange(); // window.L available because leaflet() was called in componentWillMount + this.state.currentBounds = window.L.latLngBounds(SWNE[0], SWNE[1]); this.state.map.fitBounds(window.L.latLngBounds(SWNE[0], SWNE[1])); } resetZoomButtonClicked() { From 91b1de792f91d033a2f6fcafd2ca23aa18681b33 Mon Sep 17 00:00:00 2001 From: Emma Hodcroft Date: Thu, 30 Jan 2020 18:37:54 +0100 Subject: [PATCH 03/22] add safety check to only do this in narrative mode --- src/components/map/map.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index d04442545..670b35af1 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -240,7 +240,7 @@ class Map extends React.Component { // L. available because leaflet() was called in componentWillMount this.state.currentBounds = maybeNewBounds; this.state.map.fitBounds(maybeNewBounds); - } else if (!this.state.currentBounds.equals(maybeNewBounds)) { + } else if (this.props.narrativeMode && !this.state.currentBounds.equals(maybeNewBounds)) { // check to see if the new bounds would be different for any reason - if so, change them! this.state.currentBounds = maybeNewBounds; this.state.map.fitBounds(maybeNewBounds); From 52c8de400aa108c1ecacc57f14bc32ce65b96e01 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Fri, 31 Jan 2020 14:43:56 +1300 Subject: [PATCH 04/22] [bugfix] don't unnecessarily recompute colours on narrative page changes 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. --- src/actions/recomputeReduxState.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 3607e166b..73a61cea8 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -642,7 +642,7 @@ export const createStateFromQueryOrJSONs = ({ /* calculate colours if loading from JSONs or if the query demands change */ - if (json || controls.colorBy !== oldState.colorBy) { + if (json || controls.colorBy !== oldState.controls.colorBy) { const colorScale = calcColorScale(controls.colorBy, controls, tree, treeToo, metadata); const nodeColors = calcNodeColor(tree, colorScale); controls.colorScale = colorScale; From a49e13bca7b0f664eb4e2b72fa60160c5b945b60 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Fri, 31 Jan 2020 14:55:39 +1300 Subject: [PATCH 05/22] [map] Simplify reset button functionality `maybeDrawDemes...` doesn't need to be called - all we need to do is change the map bounds without recomputing / redrawing any d3 elements --- src/components/map/map.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index 670b35af1..fc348ea3a 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -75,7 +75,6 @@ class Map extends React.Component { // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md#es6-classes this.playPauseButtonClicked = this.playPauseButtonClicked.bind(this); this.resetButtonClicked = this.resetButtonClicked.bind(this); - this.resetZoomButtonClicked = this.resetZoomButtonClicked.bind(this); this.fitMapBoundsToData = this.fitMapBoundsToData.bind(this); } @@ -560,10 +559,6 @@ class Map extends React.Component { this.state.currentBounds = window.L.latLngBounds(SWNE[0], SWNE[1]); this.state.map.fitBounds(window.L.latLngBounds(SWNE[0], SWNE[1])); } - resetZoomButtonClicked() { - this.fitMapBoundsToData(); - this.maybeDrawDemesAndTransmissions(); - } getStyles = () => { const activeResetZoomButton = true; return { @@ -587,7 +582,7 @@ class Map extends React.Component { {this.props.narrativeMode ? null : ( From 43ee2ed6220944f57b5cc2f1eecaaa639f0e208b Mon Sep 17 00:00:00 2001 From: james hadfield Date: Fri, 31 Jan 2020 15:49:52 +1300 Subject: [PATCH 06/22] [map] modify map zoom behavior 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. --- src/components/map/map.js | 101 +++++++++++++++++------- src/components/map/mapHelpersLatLong.js | 1 + 2 files changed, 73 insertions(+), 29 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index fc348ea3a..9af52de53 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -114,6 +114,7 @@ class Map extends React.Component { } } componentDidMount() { + console.log("CDM"); this.maybeChangeSize(this.props); const removed = this.maybeRemoveAllDemesAndTransmissions(this.props); /* geographic resolution just changed (ie., country to division), remove everything. this change is upstream of maybeDraw */ // TODO: if demes are color blended circles, updating rather than redrawing demes would do @@ -123,6 +124,7 @@ class Map extends React.Component { this.maybeInvalidateMapSize(this.props); } componentWillReceiveProps(nextProps) { + console.log("CWRP"); this.modulateInterfaceForNarrativeMode(nextProps); this.maybeChangeSize(nextProps); const removed = this.maybeRemoveAllDemesAndTransmissions(nextProps); /* geographic resolution just changed (ie., country to division), remove everything. this change is upstream of maybeDraw */ @@ -136,14 +138,8 @@ class Map extends React.Component { if (this.props.nodes === null) { return; } this.maybeCreateLeafletMap(); /* puts leaflet in the DOM, only done once */ this.maybeSetupD3DOMNode(); /* attaches the D3 SVG DOM node to the Leaflet DOM node, only done once */ - - /* If we are changing the geo resolution in a narrative, then we want to mimic the RESET ZOOM - button by resetting the map bounds to fit the data */ - const mapIsDrawn = !!this.state.map; - if (mapIsDrawn && this.props.narrativeMode && prevProps.geoResolution !== this.props.geoResolution) { - this.fitMapBoundsToData(); - } - this.maybeDrawDemesAndTransmissions(prevProps); /* it's the first time, or they were just removed because we changed dataset or colorby or resolution */ + console.log("CDU") + this.maybeDrawDemesAndTransmissionsAndMoveMap(prevProps); /* it's the first time, or they were just removed because we changed dataset or colorby or resolution */ } maybeInvalidateMapSize(nextProps) { /* when we procedurally change the size of the card, for instance, when we swap from grid to full */ @@ -193,7 +189,7 @@ class Map extends React.Component { } } - maybeDrawDemesAndTransmissions() { + maybeDrawDemesAndTransmissionsAndMoveMap(prevProps) { const mapIsDrawn = !!this.state.map; const allDataPresent = !!(this.props.metadata.loaded && this.props.treeLoaded && this.state.responsive && this.state.d3DOMNode); const demesTransmissionsNotComputed = !this.state.demeData && !this.state.transmissionData; @@ -206,6 +202,8 @@ class Map extends React.Component { if (mapIsDrawn && allDataPresent && demesTransmissionsNotComputed) { timerStart("drawDemesAndTransmissions"); /* data structures to feed to d3 latLongs = { tips: [{}, {}], transmissions: [{}, {}] } */ + console.log("\tDrawDemesAndTransmissions"); + const {demeData, transmissionData, demeIndices, transmissionIndices} = createDemeAndTransmissionData( this.props.nodes, @@ -221,6 +219,14 @@ class Map extends React.Component { this.props.dispatch ); + /* Now that the d3 data is created (not yet drawn) we can compute map bounds & move as appropriate */ + this.moveMapAccordingToData({ + geoResolutionChanged: prevProps.geoResolution !== this.props.geoResolution, + visibilityChanged: prevProps.visibility !== this.props.visibility, + demeData, + demeIndices + }); + // const latLongs = this.latLongs(demeData, transmissionData); /* no reference stored, we recompute this for now rather than updating in place */ const d3elems = drawDemesAndTransmissions( demeData, @@ -233,17 +239,6 @@ class Map extends React.Component { this.props.pieChart ); - const SWNE = this.getGeoRange(demeIndices, demeData); - const maybeNewBounds = L.latLngBounds(SWNE[0], SWNE[1]); - if (!this.state.boundsSet) { // we are doing the initial render -> set map to the range of the data - // L. available because leaflet() was called in componentWillMount - this.state.currentBounds = maybeNewBounds; - this.state.map.fitBounds(maybeNewBounds); - } else if (this.props.narrativeMode && !this.state.currentBounds.equals(maybeNewBounds)) { - // check to see if the new bounds would be different for any reason - if so, change them! - this.state.currentBounds = maybeNewBounds; - this.state.map.fitBounds(maybeNewBounds); - } /* Set up leaflet events */ // this.state.map.on("viewreset", this.respondToLeafletEvent.bind(this)); @@ -263,7 +258,7 @@ class Map extends React.Component { } /** * removing demes & transmissions, both from the react state & from the DOM. - * They will be created from scratch (& rendered) by `this.maybeDrawDemesAndTransmissions` + * They will be created from scratch (& rendered) by `this.maybeDrawDemesAndTransmissionsAndMoveMap` * This is done when * (a) the dataset has changed * (b) the geo resolution has changed (new transmissions, new deme locations) @@ -279,6 +274,7 @@ class Map extends React.Component { const colorByChanged = (nextProps.colorScaleVersion !== this.props.colorScaleVersion); if (mapIsDrawn && (geoResolutionChanged || dataChanged || colorByChanged)) { this.state.d3DOMNode.selectAll("*").remove(); + console.log("\tREMOVE") this.setState({ d3elems: null, demeData: null, @@ -310,20 +306,28 @@ class Map extends React.Component { this.setState({demeData: newDemes, transmissionData: newTransmissions}); } } - // Allow to pass in particular demeIndices & demeData for initial render, when these aren't officially set yet - getGeoRange(demeIndices = this.state.demeIndices, demeData = this.state.demeData) { + getGeoRange(demeData, demeIndices) { + console.log("\t\tgetGeoRange"); const latitudes = []; const longitudes = []; - // Check the count data - if it has a count of 0, it's not being drawn, so don't include in Geo range! + /* Loop through the different demes and, if they are in view (i.e. their `count` > 0) + then add their lat-longs to the the respective arrays */ this.props.metadata.geoResolutions.forEach((geoData) => { if (geoData.key === this.props.geoResolution) { const demeToLatLongs = geoData.demes; Object.keys(demeToLatLongs).forEach((deme) => { - if ((!demeIndices && !demeData) || // if we haven't loaded these yet, take all locations - (demeIndices && demeData[demeIndices[deme]].count !== 0)) { // if have, only those with counts! + if (!demeIndices || !demeData) { + /* include them all */ latitudes.push(demeToLatLongs[deme].latitude); longitudes.push(demeToLatLongs[deme].longitude); + } else { + demeIndices[deme].forEach((demeIdx) => { + if (demeData[demeIdx] && demeData[demeIdx].count > 0) { + latitudes.push(demeToLatLongs[deme].latitude); + longitudes.push(demeToLatLongs[deme].longitude); + } + }); } }); } @@ -354,6 +358,7 @@ class Map extends React.Component { if (!(visibilityChange && haveData)) { return; } + console.log("\tUpdateDemesAndTransmissions") timerStart("updateDemesAndTransmissions"); if (this.props.geoResolution !== nextProps.geoResolution) { /* This `if` statement added as part of https://github.com/nextstrain/auspice/issues/722 @@ -416,6 +421,13 @@ class Map extends React.Component { nextProps.pieChart ); + this.moveMapAccordingToData({ + geoResolutionChanged: nextProps.geoResolution !== this.props.geoResolution, + visibilityChanged: nextProps.visibility !== this.props.visibility, + demeData: newDemes, + demeIndices: this.state.demeIndices + }); + this.setState({ demeData: newDemes, transmissionData: newTransmissions @@ -553,8 +565,39 @@ class Map extends React.Component { this.props.dispatch({type: MAP_ANIMATION_PLAY_PAUSE_BUTTON, data: "Play"}); this.props.dispatch(changeDateFilter({newMin: this.props.absoluteDateMin, newMax: this.props.absoluteDateMax, quickdraw: false})); } - fitMapBoundsToData() { - const SWNE = this.getGeoRange(); + moveMapAccordingToData({geoResolutionChanged, visibilityChanged, demeData, demeIndices}) { + /* Given d3 data (may not be drawn) we can compute map bounds & move as appropriate */ + console.log("\tmoveMapAccordingToData"); + + if (!this.state.boundsSet) { + /* we are doing the initial render -> set map to the range of the data in view */ + /* P.S. This is how upon initial loading the map zooms into the data */ + this.fitMapBoundsToData(demeData, demeIndices); + return; + } + + /* if we're animating, then we don't want to move the map all the time */ + if (this.props.animationPlayPauseButton === "Pause") { + console.log("Animating -> don't move map"); + return; + } + + if (this.props.narrativeMode && geoResolutionChanged) { + /* changed geo-resolution in narrative mode => reset view */ + this.fitMapBoundsToData(demeData, demeIndices); + } else if (this.props.narrativeMode && visibilityChanged) { + /* changed visiblity (e.g. filters applied) in narrative mode => reset view */ + this.fitMapBoundsToData(demeData, demeIndices); + } + + /* TODO - in the above if / else statements, we should check if the user has interacted with the map + at all (e.g. pan / zoom). If they _haven't_ then we should treat it similarly to narrative mode & + automagically zoom to the data */ + } + + fitMapBoundsToData(demeData, demeIndices) { + console.log("\tfitMapBoundsToData"); + const SWNE = this.getGeoRange(demeData, demeIndices); // window.L available because leaflet() was called in componentWillMount this.state.currentBounds = window.L.latLngBounds(SWNE[0], SWNE[1]); this.state.map.fitBounds(window.L.latLngBounds(SWNE[0], SWNE[1])); @@ -582,7 +625,7 @@ class Map extends React.Component { {this.props.narrativeMode ? null : ( diff --git a/src/components/map/mapHelpersLatLong.js b/src/components/map/mapHelpersLatLong.js index f29cbb527..3622f4452 100644 --- a/src/components/map/mapHelpersLatLong.js +++ b/src/components/map/mapHelpersLatLong.js @@ -401,6 +401,7 @@ export const createDemeAndTransmissionData = ( colorBy, dispatch ) => { + console.log("\tcreateDemeAndTransmissionData()"); /* walk through nodes and collect all data for demeData we have: From a6e34f5fded29a1879fcd0fc6e084785acdcb08c Mon Sep 17 00:00:00 2001 From: james hadfield Date: Fri, 31 Jan 2020 16:33:17 +1300 Subject: [PATCH 07/22] [map] automatically move map if user has not panned/zoomed 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. --- src/components/map/map.js | 57 ++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index 9af52de53..ad5211ae1 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -70,7 +70,8 @@ class Map extends React.Component { demeData: null, transmissionData: null, demeIndices: null, - transmissionIndices: null + transmissionIndices: null, + userHasInteractedWithMap: false }; // https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md#es6-classes this.playPauseButtonClicked = this.playPauseButtonClicked.bind(this); @@ -289,6 +290,11 @@ class Map extends React.Component { respondToLeafletEvent(leafletEvent) { if (leafletEvent.type === "moveend") { /* zooming and panning */ + /* Movend: Fired when the center of the map stops changing (e.g. user stopped dragging the map). */ + /* Note - this method is triggered when the map sets up and is essential + for moving the d3 elements to their correct positions. It is later + triggered on pan / zoom (as you'd expect) */ + if (!this.state.demeData || !this.state.transmissionData) return; const newDemes = updateDemeDataLatLong(this.state.demeData, this.state.map); @@ -302,7 +308,6 @@ class Map extends React.Component { this.props.dateMaxNumeric, this.props.pieChart ); - this.setState({demeData: newDemes, transmissionData: newTransmissions}); } } @@ -567,8 +572,7 @@ class Map extends React.Component { } moveMapAccordingToData({geoResolutionChanged, visibilityChanged, demeData, demeIndices}) { /* Given d3 data (may not be drawn) we can compute map bounds & move as appropriate */ - console.log("\tmoveMapAccordingToData"); - + console.log("\tmoveMapAccordingToData. userHasInteractedWithMap:", this.state.userHasInteractedWithMap); if (!this.state.boundsSet) { /* we are doing the initial render -> set map to the range of the data in view */ /* P.S. This is how upon initial loading the map zooms into the data */ @@ -582,17 +586,15 @@ class Map extends React.Component { return; } - if (this.props.narrativeMode && geoResolutionChanged) { - /* changed geo-resolution in narrative mode => reset view */ - this.fitMapBoundsToData(demeData, demeIndices); - } else if (this.props.narrativeMode && visibilityChanged) { - /* changed visiblity (e.g. filters applied) in narrative mode => reset view */ - this.fitMapBoundsToData(demeData, demeIndices); + if (!this.state.userHasInteractedWithMap || this.props.narrative) { + if (geoResolutionChanged) { + /* changed geo-resolution in narrative mode => reset view */ + this.fitMapBoundsToData(demeData, demeIndices); + } else if (visibilityChanged) { + /* changed visiblity (e.g. filters applied) in narrative mode => reset view */ + this.fitMapBoundsToData(demeData, demeIndices); + } } - - /* TODO - in the above if / else statements, we should check if the user has interacted with the map - at all (e.g. pan / zoom). If they _haven't_ then we should treat it similarly to narrative mode & - automagically zoom to the data */ } fitMapBoundsToData(demeData, demeIndices) { @@ -620,17 +622,22 @@ class Map extends React.Component { const transmissionsExist = this.state.transmissionData && this.state.transmissionData.length; // clear layers - store all markers in map state https://github.com/Leaflet/Leaflet/issues/3238#issuecomment-77061011 return ( - - {this.maybeCreateMapDiv()} - {this.props.narrativeMode ? null : ( - - )} - +
{this.setState({userHasInteractedWithMap: true});}}> + + {this.maybeCreateMapDiv()} + {this.props.narrativeMode ? null : ( + + )} + +
); } } From b0365d10554bbc63c788215b4c9597429fa077cc Mon Sep 17 00:00:00 2001 From: james hadfield Date: Fri, 31 Jan 2020 17:26:05 +1300 Subject: [PATCH 08/22] [map bugfix] ensure `updateOnMoveEnd` runs on map load 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? --- src/components/map/map.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index ad5211ae1..4df60c488 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -240,11 +240,6 @@ class Map extends React.Component { this.props.pieChart ); - - /* Set up leaflet events */ - // this.state.map.on("viewreset", this.respondToLeafletEvent.bind(this)); - this.state.map.on("moveend", this.respondToLeafletEvent.bind(this)); - // don't redraw on every rerender - need to seperately handle virus change redraw this.setState({ boundsSet: true, @@ -295,11 +290,18 @@ class Map extends React.Component { for moving the d3 elements to their correct positions. It is later triggered on pan / zoom (as you'd expect) */ - if (!this.state.demeData || !this.state.transmissionData) return; + if (!this.state.demeData || !this.state.transmissionData) { + /* this seems to happen when the data takes a particularly long time to create. + and the map is ready before the data (??). It's imperitive that this method runs + so if the data's not ready yet we try to rerun it after a short time. + This could be improved */ + window.setTimeout(() => this.respondToLeafletEvent(leafletEvent), 50); + return; + } const newDemes = updateDemeDataLatLong(this.state.demeData, this.state.map); const newTransmissions = updateTransmissionDataLatLong(this.state.transmissionData, this.state.map); - + console.log("\tupdateOnMoveEnd") updateOnMoveEnd( newDemes, newTransmissions, @@ -504,6 +506,9 @@ class Map extends React.Component { L.zoomControlButtons = L.control.zoom({position: "bottomright"}).addTo(map); } + /* Set up leaflet events */ + map.on("moveend", this.respondToLeafletEvent.bind(this)); + this.setState({map}); } @@ -574,6 +579,7 @@ class Map extends React.Component { /* Given d3 data (may not be drawn) we can compute map bounds & move as appropriate */ console.log("\tmoveMapAccordingToData. userHasInteractedWithMap:", this.state.userHasInteractedWithMap); if (!this.state.boundsSet) { + console.log("\t\tSetting for the first time"); /* we are doing the initial render -> set map to the range of the data in view */ /* P.S. This is how upon initial loading the map zooms into the data */ this.fitMapBoundsToData(demeData, demeIndices); From 9a34bd3f5e77f90fe473ddef78c7b31d896cb5a5 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Sat, 1 Feb 2020 12:58:50 +1300 Subject: [PATCH 09/22] [map bugfix] clicking reset zoom resets zoom behavior also 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). --- src/components/map/map.js | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index 4df60c488..db9fa13cd 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -553,7 +553,9 @@ class Map extends React.Component { container = (
{this.animationButtons()} -
{this.setState({userHasInteractedWithMap: true});}} + id="map" style={{ height: this.state.responsive.height, width: this.state.responsive.width @@ -628,22 +630,21 @@ class Map extends React.Component { const transmissionsExist = this.state.transmissionData && this.state.transmissionData.length; // clear layers - store all markers in map state https://github.com/Leaflet/Leaflet/issues/3238#issuecomment-77061011 return ( -
{this.setState({userHasInteractedWithMap: true});}}> - - {this.maybeCreateMapDiv()} - {this.props.narrativeMode ? null : ( - - )} - -
+ + {this.maybeCreateMapDiv()} + {this.props.narrativeMode ? null : ( + + )} + ); } } From 99da19b542a26071aeeb62d8a2d09076b71aa7c7 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Sat, 1 Feb 2020 13:00:13 +1300 Subject: [PATCH 10/22] remove console logs --- src/components/map/map.js | 14 -------------- src/components/map/mapHelpersLatLong.js | 1 - 2 files changed, 15 deletions(-) diff --git a/src/components/map/map.js b/src/components/map/map.js index db9fa13cd..07ec0abd8 100644 --- a/src/components/map/map.js +++ b/src/components/map/map.js @@ -115,7 +115,6 @@ class Map extends React.Component { } } componentDidMount() { - console.log("CDM"); this.maybeChangeSize(this.props); const removed = this.maybeRemoveAllDemesAndTransmissions(this.props); /* geographic resolution just changed (ie., country to division), remove everything. this change is upstream of maybeDraw */ // TODO: if demes are color blended circles, updating rather than redrawing demes would do @@ -125,7 +124,6 @@ class Map extends React.Component { this.maybeInvalidateMapSize(this.props); } componentWillReceiveProps(nextProps) { - console.log("CWRP"); this.modulateInterfaceForNarrativeMode(nextProps); this.maybeChangeSize(nextProps); const removed = this.maybeRemoveAllDemesAndTransmissions(nextProps); /* geographic resolution just changed (ie., country to division), remove everything. this change is upstream of maybeDraw */ @@ -139,7 +137,6 @@ class Map extends React.Component { if (this.props.nodes === null) { return; } this.maybeCreateLeafletMap(); /* puts leaflet in the DOM, only done once */ this.maybeSetupD3DOMNode(); /* attaches the D3 SVG DOM node to the Leaflet DOM node, only done once */ - console.log("CDU") this.maybeDrawDemesAndTransmissionsAndMoveMap(prevProps); /* it's the first time, or they were just removed because we changed dataset or colorby or resolution */ } maybeInvalidateMapSize(nextProps) { @@ -203,8 +200,6 @@ class Map extends React.Component { if (mapIsDrawn && allDataPresent && demesTransmissionsNotComputed) { timerStart("drawDemesAndTransmissions"); /* data structures to feed to d3 latLongs = { tips: [{}, {}], transmissions: [{}, {}] } */ - console.log("\tDrawDemesAndTransmissions"); - const {demeData, transmissionData, demeIndices, transmissionIndices} = createDemeAndTransmissionData( this.props.nodes, @@ -270,7 +265,6 @@ class Map extends React.Component { const colorByChanged = (nextProps.colorScaleVersion !== this.props.colorScaleVersion); if (mapIsDrawn && (geoResolutionChanged || dataChanged || colorByChanged)) { this.state.d3DOMNode.selectAll("*").remove(); - console.log("\tREMOVE") this.setState({ d3elems: null, demeData: null, @@ -301,7 +295,6 @@ class Map extends React.Component { const newDemes = updateDemeDataLatLong(this.state.demeData, this.state.map); const newTransmissions = updateTransmissionDataLatLong(this.state.transmissionData, this.state.map); - console.log("\tupdateOnMoveEnd") updateOnMoveEnd( newDemes, newTransmissions, @@ -314,7 +307,6 @@ class Map extends React.Component { } } getGeoRange(demeData, demeIndices) { - console.log("\t\tgetGeoRange"); const latitudes = []; const longitudes = []; @@ -365,7 +357,6 @@ class Map extends React.Component { if (!(visibilityChange && haveData)) { return; } - console.log("\tUpdateDemesAndTransmissions") timerStart("updateDemesAndTransmissions"); if (this.props.geoResolution !== nextProps.geoResolution) { /* This `if` statement added as part of https://github.com/nextstrain/auspice/issues/722 @@ -579,9 +570,7 @@ class Map extends React.Component { } moveMapAccordingToData({geoResolutionChanged, visibilityChanged, demeData, demeIndices}) { /* Given d3 data (may not be drawn) we can compute map bounds & move as appropriate */ - console.log("\tmoveMapAccordingToData. userHasInteractedWithMap:", this.state.userHasInteractedWithMap); if (!this.state.boundsSet) { - console.log("\t\tSetting for the first time"); /* we are doing the initial render -> set map to the range of the data in view */ /* P.S. This is how upon initial loading the map zooms into the data */ this.fitMapBoundsToData(demeData, demeIndices); @@ -590,7 +579,6 @@ class Map extends React.Component { /* if we're animating, then we don't want to move the map all the time */ if (this.props.animationPlayPauseButton === "Pause") { - console.log("Animating -> don't move map"); return; } @@ -606,7 +594,6 @@ class Map extends React.Component { } fitMapBoundsToData(demeData, demeIndices) { - console.log("\tfitMapBoundsToData"); const SWNE = this.getGeoRange(demeData, demeIndices); // window.L available because leaflet() was called in componentWillMount this.state.currentBounds = window.L.latLngBounds(SWNE[0], SWNE[1]); @@ -637,7 +624,6 @@ class Map extends React.Component { style={{...tabSingle, ...styles.resetZoomButton}} onClick={() => { this.fitMapBoundsToData(this.state.demeData, this.state.demeIndices); - console.log("Button click") this.setState({userHasInteractedWithMap: false}); }} > diff --git a/src/components/map/mapHelpersLatLong.js b/src/components/map/mapHelpersLatLong.js index 3622f4452..f29cbb527 100644 --- a/src/components/map/mapHelpersLatLong.js +++ b/src/components/map/mapHelpersLatLong.js @@ -401,7 +401,6 @@ export const createDemeAndTransmissionData = ( colorBy, dispatch ) => { - console.log("\tcreateDemeAndTransmissionData()"); /* walk through nodes and collect all data for demeData we have: From 6e9abd3850d927c122fd47444f2104b501635a52 Mon Sep 17 00:00:00 2001 From: Emma Hodcroft Date: Wed, 29 Jan 2020 12:14:45 +0100 Subject: [PATCH 11/22] zoom by custom clade labels --- src/actions/recomputeReduxState.js | 20 ++++++++++++++- src/actions/tree.js | 6 +++++ .../tree/reactD3Interface/callbacks.js | 25 ++++++++++++++----- src/middleware/changeURL.js | 14 ++++++++++- src/util/treeVisibilityHelpers.js | 12 ++++++--- 5 files changed, 66 insertions(+), 11 deletions(-) diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 73a61cea8..05b5cce37 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -446,7 +446,7 @@ const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, cladeSelect oldState.selectedStrain = tipSelected; } if (cladeSelected) { - const cladeSelectedIdx = cladeSelected === 'root' ? 0 : getIdxMatchingLabel(oldState.nodes, "clade", cladeSelected); + const cladeSelectedIdx = cladeSelected === 'root' ? 0 : getIdxMatchingLabel(oldState.nodes, cladeSelected["label"], cladeSelected["selected"]); oldState.selectedClade = cladeSelected; newIdxRoot = applyInViewNodesToTree(cladeSelectedIdx, oldState); // tipSelectedIdx, oldState); } @@ -651,6 +651,24 @@ export const createStateFromQueryOrJSONs = ({ tree.nodeColors = nodeColors; } + // 'clade' zoom can now be under any label - check for first available query key that + // matches available branch labels, and convert it to be 'query.clade' + // If the 'label' doesnt exist on the tree (ex: in label=clade:T, if 'clade' doesn't exist) then + // the URL is cleared as well as the tree doing nothing. + // However, if the 'selection' doesnt exist on the tree (ex: in label=clade:T, if 'clade' exists but 'T' doesnt) + // then the tree does nothing and the URL is not cleared. (This is current behaviour.) + const queryKeys = Object.keys(query); + if (queryKeys.includes("label")) { + const label_parts = query["label"].split(":"); + const possibleClade = label_parts[0]; + if (tree.availableBranchLabels.includes(possibleClade)) { + query.clade = {label: possibleClade, selected: label_parts[1]}; + } else { + query.clade = undefined; + delete query["label"]; + } + } + if (query.clade) { tree = modifyTreeStateVisAndBranchThickness(tree, undefined, query.clade, controls); } else { /* if not specifically given in URL, zoom to root */ diff --git a/src/actions/tree.js b/src/actions/tree.js index e68c0622c..214510dbf 100644 --- a/src/actions/tree.js +++ b/src/actions/tree.js @@ -77,6 +77,12 @@ export const updateVisibleTipsAndBranchThicknesses = ( ) => { return (dispatch, getState) => { const { tree, treeToo, controls, frequencies } = getState(); + // If going back to root or to unlabelled branch from 'custom' label branch, need to + // expliticly set that 'label' to undefined, so it gets deleted from URL! + if (tree.selectedClade && !cladeSelected) { + cladeSelected = tree.selectedClade; + cladeSelected["selected"] = undefined; + } if (root[0] === undefined && !cladeSelected && tree.selectedClade) { /* if not resetting tree to root, maintain previous selectedClade if one exists */ cladeSelected = tree.selectedClade; diff --git a/src/components/tree/reactD3Interface/callbacks.js b/src/components/tree/reactD3Interface/callbacks.js index 6eaa006af..030a8a61b 100644 --- a/src/components/tree/reactD3Interface/callbacks.js +++ b/src/components/tree/reactD3Interface/callbacks.js @@ -75,12 +75,25 @@ export const onBranchClick = function onBranchClick(d) { if (this.props.narrativeMode) return; const root = [undefined, undefined]; let cladeSelected; - if ( - d.n.branch_attrs && - d.n.branch_attrs.labels !== undefined && - d.n.branch_attrs.labels.clade !== undefined - ) { - cladeSelected = d.n.branch_attrs.labels.clade; + // Branches with multiple labels will be used in the order specified by this.props.tree.availableBranchLabels + // (The order of the drop-down on the menu) + // Can't use AA mut lists as zoom labels currently - URL is bad, but also, means every node has a label, and many conflict... + let legalBranchLabels; + // Check has some branch labels, and remove 'aa' ones. + if (d.n.branch_attrs && + d.n.branch_attrs.labels !== undefined) { + legalBranchLabels = Object.keys(d.n.branch_attrs.labels).filter((label) => label !== "aa"); + } + // If has some, then could be clade label - but sort first + if (legalBranchLabels) { + const availableBranchLabels = this.props.tree.availableBranchLabels; + // sort the possible branch labels by the order of those available on the tree + legalBranchLabels.sort((a, b) => + availableBranchLabels.indexOf(a) - availableBranchLabels.indexOf(b) + ); + // then use the first! + const key = legalBranchLabels[0]; + cladeSelected = {label: key, selected: d.n.branch_attrs.labels[key]}; } if (d.that.params.orientation[0] === 1) root[0] = d.n.arrayIdx; else root[1] = d.n.arrayIdx; diff --git a/src/middleware/changeURL.js b/src/middleware/changeURL.js index c736691cd..c84004e1e 100644 --- a/src/middleware/changeURL.js +++ b/src/middleware/changeURL.js @@ -175,8 +175,20 @@ export const changeURLMiddleware = (store) => (next) => (action) => { } /* small modifications to desired pathname / query */ + // Custom labels come in under 'query.clade' but have their own label (may not be 'clade') + // to act appropriately, put them in 'query' under their own 'label'. + // Ex: query.clade = {label: animal, selected: dog} becomes query.label = animal:dog + // Need to run this whether it's from a click event ('clade') or from a URL load ('label') + if ((query["clade"] && query["clade"] !== "" && query["clade"]["label"]) || + (query["label"] && query["label"] !== "")) { + const label_parts = query["label"] ? query["label"].split(":") : ""; + const label = query["clade"] ? query["clade"]["label"] : label_parts[0]; + const selected = query["clade"] ? query["clade"]["selected"] : label_parts[1]; + delete query["clade"]; // get rid of query.clade + query["label"] = selected ? ""+label+":"+selected : selected; + } Object.keys(query).filter((q) => query[q] === "").forEach((k) => delete query[k]); - let search = queryString.stringify(query).replace(/%2C/g, ',').replace(/%2F/g, '/'); + let search = queryString.stringify(query).replace(/%2C/g, ',').replace(/%2F/g, '/').replace(/%3A/g, ':'); if (search) {search = "?" + search;} if (!pathname.startsWith("/")) {pathname = "/" + pathname;} diff --git a/src/util/treeVisibilityHelpers.js b/src/util/treeVisibilityHelpers.js index 941615eb4..6f56cbc1e 100644 --- a/src/util/treeVisibilityHelpers.js +++ b/src/util/treeVisibilityHelpers.js @@ -32,17 +32,23 @@ export const strainNameToIdx = (nodes, name) => { */ export const getIdxMatchingLabel = (nodes, labelName, labelValue) => { let i; + let found = 0; for (i = 0; i < nodes.length; i++) { if ( nodes[i].branch_attrs && nodes[i].branch_attrs.labels !== undefined && nodes[i].branch_attrs.labels[labelName] === labelValue ) { - return i; + if (found === 0) { + found = i; + } else { + console.error(`getIdxMatchingLabel found multiple labels ${labelName}===${labelValue}`); + return 0; + } } } - console.error(`getIdxMatchingLabel couldn't find label ${labelName}===${labelValue}`); - return 0; + if (found === 0) { console.error(`getIdxMatchingLabel couldn't find label ${labelName}===${labelValue}`); } + return found; }; /** calcBranchThickness ** From 96e319b8885f95821716e95ac67bf74325b4bb98 Mon Sep 17 00:00:00 2001 From: Emma Hodcroft Date: Wed, 29 Jan 2020 13:49:26 +0100 Subject: [PATCH 12/22] add pop up messages for invalid URL zoom requests --- src/actions/loadData.js | 5 +++-- src/actions/recomputeReduxState.js | 20 +++++++++++--------- src/util/treeVisibilityHelpers.js | 15 +++++++++++++-- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/actions/loadData.js b/src/actions/loadData.js index 91db7b6cc..0b24d4742 100644 --- a/src/actions/loadData.js +++ b/src/actions/loadData.js @@ -181,7 +181,8 @@ const fetchDataAndDispatch = async (dispatch, url, query, narrativeBlocks) => { query, narrativeBlocks, mainTreeName: secondTreeUrl ? mainDatasetUrl : null, - secondTreeName: secondTreeUrl ? secondTreeUrl : null + secondTreeName: secondTreeUrl ? secondTreeUrl : null, + dispatch }) }); @@ -233,7 +234,7 @@ export const loadSecondTree = (secondTreeUrl, firstTreeUrl) => async (dispatch, return; } const oldState = getState(); - const newState = createTreeTooState({treeTooJSON: secondJson.tree, oldState, originalTreeUrl: firstTreeUrl, secondTreeUrl: secondTreeUrl}); + const newState = createTreeTooState({treeTooJSON: secondJson.tree, oldState, originalTreeUrl: firstTreeUrl, secondTreeUrl: secondTreeUrl, dispatch}); dispatch({type: types.TREE_TOO_DATA, ...newState}); }; diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 05b5cce37..fb369e09e 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -436,7 +436,7 @@ const checkAndCorrectErrorsInState = (state, metadata, query, tree) => { return state; }; -const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, cladeSelected, controlsState) => { +const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, cladeSelected, controlsState, dispatch) => { /* calculate new branch thicknesses & visibility */ let tipSelectedIdx = 0; /* check if the query defines a strain to be selected */ @@ -446,7 +446,7 @@ const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, cladeSelect oldState.selectedStrain = tipSelected; } if (cladeSelected) { - const cladeSelectedIdx = cladeSelected === 'root' ? 0 : getIdxMatchingLabel(oldState.nodes, cladeSelected["label"], cladeSelected["selected"]); + const cladeSelectedIdx = cladeSelected === 'root' ? 0 : getIdxMatchingLabel(oldState.nodes, cladeSelected["label"], cladeSelected["selected"], dispatch); oldState.selectedClade = cladeSelected; newIdxRoot = applyInViewNodesToTree(cladeSelectedIdx, oldState); // tipSelectedIdx, oldState); } @@ -574,7 +574,8 @@ export const createStateFromQueryOrJSONs = ({ narrativeBlocks = false, mainTreeName = false, secondTreeName = false, - query + query, + dispatch }) => { let tree, treeToo, entropy, controls, metadata, narrative, frequencies; /* first task is to create metadata, entropy, controls & tree partial state */ @@ -670,15 +671,15 @@ export const createStateFromQueryOrJSONs = ({ } if (query.clade) { - tree = modifyTreeStateVisAndBranchThickness(tree, undefined, query.clade, controls); + tree = modifyTreeStateVisAndBranchThickness(tree, undefined, query.clade, controls, dispatch); } else { /* if not specifically given in URL, zoom to root */ - tree = modifyTreeStateVisAndBranchThickness(tree, undefined, undefined, controls); + tree = modifyTreeStateVisAndBranchThickness(tree, undefined, undefined, controls, dispatch); } - tree = modifyTreeStateVisAndBranchThickness(tree, query.s, undefined, controls); + tree = modifyTreeStateVisAndBranchThickness(tree, query.s, undefined, controls, dispatch); if (treeToo && treeToo.loaded) { treeToo.nodeColorsVersion = tree.nodeColorsVersion; treeToo.nodeColors = calcNodeColor(treeToo, controls.colorScale); - treeToo = modifyTreeStateVisAndBranchThickness(treeToo, query.s, undefined, controls); + treeToo = modifyTreeStateVisAndBranchThickness(treeToo, query.s, undefined, controls, dispatch); controls = modifyControlsViaTreeToo(controls, treeToo.name); treeToo.tangleTipLookup = constructVisibleTipLookupBetweenTrees(tree.nodes, treeToo.nodes, tree.visibility, treeToo.visibility); } @@ -713,7 +714,8 @@ export const createTreeTooState = ({ treeTooJSON, /* raw json data */ oldState, originalTreeUrl, - secondTreeUrl /* treeToo URL */ + secondTreeUrl, /* treeToo URL */ + dispatch }) => { /* TODO: reconsile choices (filters, colorBys etc) with this new tree */ /* TODO: reconcile query with visibility etc */ @@ -725,7 +727,7 @@ export const createTreeTooState = ({ treeToo.debug = "RIGHT"; controls = modifyControlsStateViaTree(controls, tree, treeToo, oldState.metadata.colorings); controls = modifyControlsViaTreeToo(controls, secondTreeUrl); - treeToo = modifyTreeStateVisAndBranchThickness(treeToo, tree.selectedStrain, undefined, controls); + treeToo = modifyTreeStateVisAndBranchThickness(treeToo, tree.selectedStrain, undefined, controls, dispatch); /* calculate colours if loading from JSONs or if the query demands change */ const colorScale = calcColorScale(controls.colorBy, controls, tree, treeToo, oldState.metadata); diff --git a/src/util/treeVisibilityHelpers.js b/src/util/treeVisibilityHelpers.js index 6f56cbc1e..ad33a41aa 100644 --- a/src/util/treeVisibilityHelpers.js +++ b/src/util/treeVisibilityHelpers.js @@ -1,6 +1,7 @@ import { freqScale, NODE_NOT_VISIBLE, NODE_VISIBLE_TO_MAP_ONLY, NODE_VISIBLE } from "./globals"; import { calcTipCounts } from "./treeCountingHelpers"; import { getTraitFromNode } from "./treeMiscHelpers"; +import { warningNotification } from "../actions/notifications"; export const getVisibleDateRange = (nodes, visibility) => nodes .filter((node, idx) => (visibility[idx] === NODE_VISIBLE && !node.hasChildren)) @@ -30,7 +31,7 @@ export const strainNameToIdx = (nodes, name) => { * @param {string} labelValue label value * @returns {int} the index of the matching node (0 if no match found) */ -export const getIdxMatchingLabel = (nodes, labelName, labelValue) => { +export const getIdxMatchingLabel = (nodes, labelName, labelValue, dispatch) => { let i; let found = 0; for (i = 0; i < nodes.length; i++) { @@ -43,11 +44,21 @@ export const getIdxMatchingLabel = (nodes, labelName, labelValue) => { found = i; } else { console.error(`getIdxMatchingLabel found multiple labels ${labelName}===${labelValue}`); + dispatch(warningNotification({ + message: "Specified Zoom Label Found Multiple Times!", + details: "Multiple nodes in the tree are labelled '"+labelName+" "+labelValue+"' - no zoom performed" + })); return 0; } } } - if (found === 0) { console.error(`getIdxMatchingLabel couldn't find label ${labelName}===${labelValue}`); } + if (found === 0) { + console.error(`getIdxMatchingLabel couldn't find label ${labelName}===${labelValue}`); + dispatch(warningNotification({ + message: "Specified Zoom Label Value Not Found!", + details: "The label '"+labelName+"' value '"+labelValue+"' was not found in the tree - no zoom performed" + })); + } return found; }; From 1a31df61e0f634461d877e8b552d435575828bb1 Mon Sep 17 00:00:00 2001 From: Emma Hodcroft Date: Mon, 3 Feb 2020 18:06:32 +0100 Subject: [PATCH 13/22] add backwards compat & stop bad URL when no label --- src/actions/recomputeReduxState.js | 11 +++++++++-- src/middleware/changeURL.js | 15 ++++++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index fb369e09e..8c7a719a7 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -446,8 +446,15 @@ const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, cladeSelect oldState.selectedStrain = tipSelected; } if (cladeSelected) { - const cladeSelectedIdx = cladeSelected === 'root' ? 0 : getIdxMatchingLabel(oldState.nodes, cladeSelected["label"], cladeSelected["selected"], dispatch); - oldState.selectedClade = cladeSelected; + // Check and fix old format 'clade=B' - in this case selectionClade is just 'B' + let safeCladeSelected = {}; + if (typeof cladeSelected === "string" && cladeSelected !== 'root') { + safeCladeSelected = {label: "clade", selected: cladeSelected}; + } else { + safeCladeSelected = cladeSelected; + } + const cladeSelectedIdx = safeCladeSelected === 'root' ? 0 : getIdxMatchingLabel(oldState.nodes, safeCladeSelected["label"], safeCladeSelected["selected"], dispatch); + oldState.selectedClade = safeCladeSelected; newIdxRoot = applyInViewNodesToTree(cladeSelectedIdx, oldState); // tipSelectedIdx, oldState); } const visAndThicknessData = calculateVisiblityAndBranchThickness( diff --git a/src/middleware/changeURL.js b/src/middleware/changeURL.js index c84004e1e..0c6231d36 100644 --- a/src/middleware/changeURL.js +++ b/src/middleware/changeURL.js @@ -179,11 +179,20 @@ export const changeURLMiddleware = (store) => (next) => (action) => { // to act appropriately, put them in 'query' under their own 'label'. // Ex: query.clade = {label: animal, selected: dog} becomes query.label = animal:dog // Need to run this whether it's from a click event ('clade') or from a URL load ('label') - if ((query["clade"] && query["clade"] !== "" && query["clade"]["label"]) || + if ((query["clade"] && query["clade"] !== "") || (query["label"] && query["label"] !== "")) { const label_parts = query["label"] ? query["label"].split(":") : ""; - const label = query["clade"] ? query["clade"]["label"] : label_parts[0]; - const selected = query["clade"] ? query["clade"]["selected"] : label_parts[1]; + + let label; + let selected; + // If old format ('?clade=B') need to recognise and act appropriately + if (typeof query["clade"] === 'string') { + label = "clade"; + selected = query["clade"]; + } else { + label = query["clade"] ? query["clade"]["label"] : label_parts[0]; + selected = query["clade"] ? query["clade"]["selected"] : label_parts[1]; + } delete query["clade"]; // get rid of query.clade query["label"] = selected ? ""+label+":"+selected : selected; } From b1a3f85a863c3fec2026127dee4300c0e7c3b3a7 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Tue, 4 Feb 2020 17:37:21 +1300 Subject: [PATCH 14/22] use query.label=: throughout code 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 `:` 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. --- src/actions/recomputeReduxState.js | 55 ++++++++----------- src/actions/tree.js | 6 -- .../tree/reactD3Interface/callbacks.js | 2 +- src/middleware/changeURL.js | 24 +------- 4 files changed, 26 insertions(+), 61 deletions(-) diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 8c7a719a7..26c343e77 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -436,7 +436,7 @@ const checkAndCorrectErrorsInState = (state, metadata, query, tree) => { return state; }; -const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, cladeSelected, controlsState, dispatch) => { +const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, zoomSelected, controlsState, dispatch) => { /* calculate new branch thicknesses & visibility */ let tipSelectedIdx = 0; /* check if the query defines a strain to be selected */ @@ -445,17 +445,15 @@ const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, cladeSelect tipSelectedIdx = strainNameToIdx(oldState.nodes, tipSelected); oldState.selectedStrain = tipSelected; } - if (cladeSelected) { + if (zoomSelected) { // Check and fix old format 'clade=B' - in this case selectionClade is just 'B' - let safeCladeSelected = {}; - if (typeof cladeSelected === "string" && cladeSelected !== 'root') { - safeCladeSelected = {label: "clade", selected: cladeSelected}; - } else { - safeCladeSelected = cladeSelected; - } - const cladeSelectedIdx = safeCladeSelected === 'root' ? 0 : getIdxMatchingLabel(oldState.nodes, safeCladeSelected["label"], safeCladeSelected["selected"], dispatch); - oldState.selectedClade = safeCladeSelected; + const [labelName, labelValue] = zoomSelected.split(":"); + const cladeSelectedIdx = getIdxMatchingLabel(oldState.nodes, labelName, labelValue, dispatch); + oldState.selectedClade = zoomSelected; newIdxRoot = applyInViewNodesToTree(cladeSelectedIdx, oldState); // tipSelectedIdx, oldState); + } else { + oldState.selectedClade = undefined; + newIdxRoot = applyInViewNodesToTree(0, oldState); // tipSelectedIdx, oldState); } const visAndThicknessData = calculateVisiblityAndBranchThickness( oldState, @@ -659,30 +657,25 @@ export const createStateFromQueryOrJSONs = ({ tree.nodeColors = nodeColors; } - // 'clade' zoom can now be under any label - check for first available query key that - // matches available branch labels, and convert it to be 'query.clade' - // If the 'label' doesnt exist on the tree (ex: in label=clade:T, if 'clade' doesn't exist) then - // the URL is cleared as well as the tree doing nothing. - // However, if the 'selection' doesnt exist on the tree (ex: in label=clade:T, if 'clade' exists but 'T' doesnt) - // then the tree does nothing and the URL is not cleared. (This is current behaviour.) - const queryKeys = Object.keys(query); - if (queryKeys.includes("label")) { - const label_parts = query["label"].split(":"); - const possibleClade = label_parts[0]; - if (tree.availableBranchLabels.includes(possibleClade)) { - query.clade = {label: possibleClade, selected: label_parts[1]}; - } else { - query.clade = undefined; - delete query["label"]; + /* parse the query.label / query.clade */ + if (query.clade) { + if (!query.label) query.label = `clade:${query.clade}`; + delete query.clade; + } + if (query.label) { + if (!query.label.includes(":")) { + console.error("Defined a label without ':' separator."); + delete query.label; + } + if (!tree.availableBranchLabels.includes(query.label.split(":")[0])) { + console.error(`Label name ${query.label.split(":")[0]} doesn't exist`); + delete query.label; } } - if (query.clade) { - tree = modifyTreeStateVisAndBranchThickness(tree, undefined, query.clade, controls, dispatch); - } else { /* if not specifically given in URL, zoom to root */ - tree = modifyTreeStateVisAndBranchThickness(tree, undefined, undefined, controls, dispatch); - } - tree = modifyTreeStateVisAndBranchThickness(tree, query.s, undefined, controls, dispatch); + /* if query.label is undefined then we intend to zoom to the root */ + tree = modifyTreeStateVisAndBranchThickness(tree, query.s, query.label, controls, dispatch); + if (treeToo && treeToo.loaded) { treeToo.nodeColorsVersion = tree.nodeColorsVersion; treeToo.nodeColors = calcNodeColor(treeToo, controls.colorScale); diff --git a/src/actions/tree.js b/src/actions/tree.js index 214510dbf..e68c0622c 100644 --- a/src/actions/tree.js +++ b/src/actions/tree.js @@ -77,12 +77,6 @@ export const updateVisibleTipsAndBranchThicknesses = ( ) => { return (dispatch, getState) => { const { tree, treeToo, controls, frequencies } = getState(); - // If going back to root or to unlabelled branch from 'custom' label branch, need to - // expliticly set that 'label' to undefined, so it gets deleted from URL! - if (tree.selectedClade && !cladeSelected) { - cladeSelected = tree.selectedClade; - cladeSelected["selected"] = undefined; - } if (root[0] === undefined && !cladeSelected && tree.selectedClade) { /* if not resetting tree to root, maintain previous selectedClade if one exists */ cladeSelected = tree.selectedClade; diff --git a/src/components/tree/reactD3Interface/callbacks.js b/src/components/tree/reactD3Interface/callbacks.js index 030a8a61b..c62da3980 100644 --- a/src/components/tree/reactD3Interface/callbacks.js +++ b/src/components/tree/reactD3Interface/callbacks.js @@ -93,7 +93,7 @@ export const onBranchClick = function onBranchClick(d) { ); // then use the first! const key = legalBranchLabels[0]; - cladeSelected = {label: key, selected: d.n.branch_attrs.labels[key]}; + cladeSelected = `${key}:${d.n.branch_attrs.labels[key]}`; } if (d.that.params.orientation[0] === 1) root[0] = d.n.arrayIdx; else root[1] = d.n.arrayIdx; diff --git a/src/middleware/changeURL.js b/src/middleware/changeURL.js index 0c6231d36..ab24bc860 100644 --- a/src/middleware/changeURL.js +++ b/src/middleware/changeURL.js @@ -88,7 +88,7 @@ export const changeURLMiddleware = (store) => (next) => (action) => { } case types.UPDATE_VISIBILITY_AND_BRANCH_THICKNESS: { query.s = action.selectedStrain ? action.selectedStrain : undefined; - query.clade = action.cladeName ? action.cladeName : undefined; + query.label = action.cladeName ? action.cladeName : undefined; break; } case types.MAP_ANIMATION_PLAY_PAUSE_BUTTON: @@ -174,28 +174,6 @@ export const changeURLMiddleware = (store) => (next) => (action) => { break; } - /* small modifications to desired pathname / query */ - // Custom labels come in under 'query.clade' but have their own label (may not be 'clade') - // to act appropriately, put them in 'query' under their own 'label'. - // Ex: query.clade = {label: animal, selected: dog} becomes query.label = animal:dog - // Need to run this whether it's from a click event ('clade') or from a URL load ('label') - if ((query["clade"] && query["clade"] !== "") || - (query["label"] && query["label"] !== "")) { - const label_parts = query["label"] ? query["label"].split(":") : ""; - - let label; - let selected; - // If old format ('?clade=B') need to recognise and act appropriately - if (typeof query["clade"] === 'string') { - label = "clade"; - selected = query["clade"]; - } else { - label = query["clade"] ? query["clade"]["label"] : label_parts[0]; - selected = query["clade"] ? query["clade"]["selected"] : label_parts[1]; - } - delete query["clade"]; // get rid of query.clade - query["label"] = selected ? ""+label+":"+selected : selected; - } Object.keys(query).filter((q) => query[q] === "").forEach((k) => delete query[k]); let search = queryString.stringify(query).replace(/%2C/g, ',').replace(/%2F/g, '/').replace(/%3A/g, ':'); if (search) {search = "?" + search;} From 125206d966fe8ebe5f1da825e88bca68ee6fde65 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Tue, 4 Feb 2020 17:51:10 +1300 Subject: [PATCH 15/22] fix bug resulting in URL query `?label=undefined:undefined` --- src/components/tree/reactD3Interface/callbacks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/tree/reactD3Interface/callbacks.js b/src/components/tree/reactD3Interface/callbacks.js index c62da3980..633e85f2e 100644 --- a/src/components/tree/reactD3Interface/callbacks.js +++ b/src/components/tree/reactD3Interface/callbacks.js @@ -85,7 +85,7 @@ export const onBranchClick = function onBranchClick(d) { legalBranchLabels = Object.keys(d.n.branch_attrs.labels).filter((label) => label !== "aa"); } // If has some, then could be clade label - but sort first - if (legalBranchLabels) { + if (legalBranchLabels && legalBranchLabels.length) { const availableBranchLabels = this.props.tree.availableBranchLabels; // sort the possible branch labels by the order of those available on the tree legalBranchLabels.sort((a, b) => From 0f7a397afe0a8e62fd585612b685697fc571ef56 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Tue, 4 Feb 2020 18:41:24 +1300 Subject: [PATCH 16/22] Allow URL and JSON to define which branch label is displayed 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. --- .../advanced-functionality/view-settings.md | 4 +++- src/actions/recomputeReduxState.js | 24 ++++++++++++++++--- src/middleware/changeURL.js | 5 ++++ src/reducers/controls.js | 5 ++-- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/docs-src/docs/advanced-functionality/view-settings.md b/docs-src/docs/advanced-functionality/view-settings.md index 8ca2b5274..62aa5aa7b 100644 --- a/docs-src/docs/advanced-functionality/view-settings.md +++ b/docs-src/docs/advanced-functionality/view-settings.md @@ -20,7 +20,7 @@ Each of these can be overridden by the JSON `display_defaults`, and then the vie * Default phylogeny distance measure is time, if available. * Default geographic resolution is "country", if available. * Default colouring is "country", if available. - +* Default branch labelling is "clade", if available. ## Dataset (JSON) configurable defaults @@ -35,6 +35,7 @@ For instance, if you set `display_defaults.color_by` to `country`, but load the | `distance_measure` | Phylogeny x-axis measure | "div" or "num_date" | | `map_triplicate` | Should the map repeat, so that you can pan further in each direction? | Boolean | | `layout` | Tree layout | "rect", "radial", "clock" or "unrooted | +| `branch_label` | Which set of branch labels are to be displayed | "aa", "lineage" | Furthermore, a JSON property `meta.panels` lists which panels auspice displays. If this is not included, then auspice tries to display as many as possible. @@ -66,6 +67,7 @@ All URL queries modify the view away from the default settings -- if you change | `animate` | Animation settings | | | `n` | Narrative page number | `n=1` goes to the first page | | `s` | Selected strain | `s=1_0199_PF` | +| `branchLabel` | Branch labels to display | `branchLabel=aa` | | `clade` | Labeled clade that tree is zoomed to | `clade=B3` (numeric values are buggy) | diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 26c343e77..626e9cea6 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -100,6 +100,10 @@ const modifyStateViaURLQuery = (state, query) => { } else { state.animationPlayPauseButton = "Play"; } + if (query.branchLabel) { + state.selectedBranchLabel = query.branchLabel; + // do not modify the default (only the JSON can do this) + } return state; }; @@ -164,8 +168,8 @@ const modifyStateViaMetadata = (state, metadata) => { console.warn("JSON did not include any filters"); } if (metadata.displayDefaults) { - const keysToCheckFor = ["geoResolution", "colorBy", "distanceMeasure", "layout", "mapTriplicate"]; - const expectedTypes = ["string", "string", "string", "string", "boolean"]; + const keysToCheckFor = ["geoResolution", "colorBy", "distanceMeasure", "layout", "mapTriplicate", "selectedBranchLabel"]; + const expectedTypes = ["string", "string", "string", "string", "boolean", "string"]; for (let i = 0; i < keysToCheckFor.length; i += 1) { if (metadata.displayDefaults[keysToCheckFor[i]]) { @@ -310,7 +314,13 @@ const modifyControlsStateViaTree = (state, tree, treeToo, colorings) => { state.distanceMeasure = state.branchLengthsToDisplay === "divOnly" ? "div" : state.branchLengthsToDisplay === "dateOnly" ? "num_date" : state.distanceMeasure; - state.selectedBranchLabel = tree.availableBranchLabels.indexOf("clade") !== -1 ? "clade" : "none"; + /* if clade is available as a branch label, then set this as the "default". This + is largely due to historical reasons. Note that it *can* and *will* be overridden + by JSON display_defaults and URL query */ + if (tree.availableBranchLabels.indexOf("clade") !== -1) { + state.defaults.selectedBranchLabel = "clade"; + state.selectedBranchLabel = "clade"; + } state.temporalConfidence = getTraitFromNode(tree.nodes[0], "num_date", {confidence: true}) ? {exists: true, display: true, on: false} : @@ -395,6 +405,13 @@ const checkAndCorrectErrorsInState = (state, metadata, query, tree) => { console.warn("JSONs did not include `geoResolutions`"); } + /* show label */ + if (state.selectedBranchLabel && !tree.availableBranchLabels.includes(state.selectedBranchLabel)) { + console.error("Can't set selected branch label to ", state.selectedBranchLabel); + state.selectedBranchLabel = "none"; + state.defaults.selectedBranchLabel = "none"; + } + /* temporalConfidence */ if (state.temporalConfidence.exists) { if (state.layout !== "rect") { @@ -551,6 +568,7 @@ const createMetadataStateFromJSON = (json) => { color_by: "colorBy", geo_resolution: "geoResolution", distance_measure: "distanceMeasure", + branch_label: "selectedBranchLabel", map_triplicate: "mapTriplicate", layout: "layout" }; diff --git a/src/middleware/changeURL.js b/src/middleware/changeURL.js index ab24bc860..288e17c71 100644 --- a/src/middleware/changeURL.js +++ b/src/middleware/changeURL.js @@ -39,6 +39,11 @@ export const changeURLMiddleware = (store) => (next) => (action) => { if (query.n === 0) delete query.n; if (query.tt) delete query.tt; break; + case types.CHANGE_BRANCH_LABEL: + query.branchLabel = state.controls.defaults.selectedBranchLabel === action.value ? + undefined : + action.value; + break; case types.CHANGE_ZOOM: /* entropy panel genome zoom coordinates */ query.gmin = action.zoomc[0] === state.controls.absoluteZoomMin ? undefined : action.zoomc[0]; diff --git a/src/reducers/controls.js b/src/reducers/controls.js index d1bf3eeea..74442bc33 100644 --- a/src/reducers/controls.js +++ b/src/reducers/controls.js @@ -19,7 +19,8 @@ export const getDefaultControlsState = () => { layout: defaultLayout, geoResolution: defaultGeoResolution, filters: {}, - colorBy: defaultColorBy + colorBy: defaultColorBy, + selectedBranchLabel: "none" }; const dateMin = numericToCalendar(currentNumDate() - defaultDateRange); const dateMax = currentCalDate(); @@ -50,7 +51,7 @@ export const getDefaultControlsState = () => { colorBy: defaults.colorBy, colorByConfidence: {display: false, on: false}, colorScale: undefined, - selectedBranchLabel: false, + selectedBranchLabel: "none", analysisSlider: false, geoResolution: defaults.geoResolution, filters: {}, From 709f79334099d6e0fbb95fafded386f3db10d201 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Tue, 4 Feb 2020 19:06:31 +1300 Subject: [PATCH 17/22] clarify code usage re: calculating branch thickness 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. --- src/actions/recomputeReduxState.js | 2 +- src/actions/tree.js | 4 ++-- src/util/treeVisibilityHelpers.js | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 626e9cea6..462d1c4cd 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -476,7 +476,7 @@ const modifyTreeStateVisAndBranchThickness = (oldState, tipSelected, zoomSelecte oldState, controlsState, {dateMinNumeric: controlsState.dateMinNumeric, dateMaxNumeric: controlsState.dateMaxNumeric}, - {tipSelectedIdx, validIdxRoot: newIdxRoot} + {tipSelectedIdx} ); const newState = Object.assign({}, oldState, visAndThicknessData); diff --git a/src/actions/tree.js b/src/actions/tree.js index e68c0622c..455e0a218 100644 --- a/src/actions/tree.js +++ b/src/actions/tree.js @@ -92,7 +92,7 @@ export const updateVisibleTipsAndBranchThicknesses = ( tree, controls, {dateMinNumeric: controls.dateMinNumeric, dateMaxNumeric: controls.dateMaxNumeric}, - {tipSelectedIdx: tipIdx1, validIdxRoot: rootIdxTree1} + {tipSelectedIdx: tipIdx1} ); const dispatchObj = { type: types.UPDATE_VISIBILITY_AND_BRANCH_THICKNESS, @@ -113,7 +113,7 @@ export const updateVisibleTipsAndBranchThicknesses = ( treeToo, controls, {dateMinNumeric: controls.dateMinNumeric, dateMaxNumeric: controls.dateMaxNumeric}, - {tipSelectedIdx: tipIdx2, validIdxRoot: rootIdxTree2} + {tipSelectedIdx: tipIdx2} ); dispatchObj.tangleTipLookup = constructVisibleTipLookupBetweenTrees(tree.nodes, treeToo.nodes, data.visibility, dataToo.visibility); dispatchObj.visibilityToo = dataToo.visibility; diff --git a/src/util/treeVisibilityHelpers.js b/src/util/treeVisibilityHelpers.js index ad33a41aa..dc50d0272 100644 --- a/src/util/treeVisibilityHelpers.js +++ b/src/util/treeVisibilityHelpers.js @@ -65,14 +65,14 @@ export const getIdxMatchingLabel = (nodes, labelName, labelValue, dispatch) => { /** calcBranchThickness ** * returns an array of node (branch) thicknesses based on the tipCount at each node * If the node isn't visible, the thickness is 1. +* Relies on the `tipCount` property of the nodes having been updated. * Pure. * @param nodes - JSON nodes * @param visibility - visibility array (1-1 with nodes) -* @param rootIdx - nodes index of the currently in-view root * @returns array of thicknesses (numeric) */ -const calcBranchThickness = (nodes, visibility, rootIdx) => { - let maxTipCount = nodes[rootIdx].tipCount; +const calcBranchThickness = (nodes, visibility) => { + let maxTipCount = nodes[0].tipCount; /* edge case: no tips selected */ if (!maxTipCount) { maxTipCount = 1; @@ -197,7 +197,7 @@ const calcVisibility = (tree, controls, dates) => { return NODE_VISIBLE; }; -export const calculateVisiblityAndBranchThickness = (tree, controls, dates, {idxOfInViewRootNode = 0, tipSelectedIdx = 0} = {}) => { +export const calculateVisiblityAndBranchThickness = (tree, controls, dates, {tipSelectedIdx = 0} = {}) => { const visibility = tipSelectedIdx ? identifyPathToTip(tree.nodes, tipSelectedIdx) : calcVisibility(tree, controls, dates); /* recalculate tipCounts over the tree - modifies redux tree nodes in place (yeah, I know) */ calcTipCounts(tree.nodes[0], visibility); @@ -205,7 +205,7 @@ export const calculateVisiblityAndBranchThickness = (tree, controls, dates, {idx return { visibility: visibility, visibilityVersion: tree.visibilityVersion + 1, - branchThickness: calcBranchThickness(tree.nodes, visibility, idxOfInViewRootNode), + branchThickness: calcBranchThickness(tree.nodes, visibility), branchThicknessVersion: tree.branchThicknessVersion + 1 }; }; From b69c708506dc4ad51598a8aec0ba7f31cae83c5c Mon Sep 17 00:00:00 2001 From: james hadfield Date: Wed, 5 Feb 2020 09:19:31 +1300 Subject: [PATCH 18/22] bugfix: ensure `dispatch` available for `createStateFromQueryOrJSONs` 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. --- src/actions/navigation.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/actions/navigation.js b/src/actions/navigation.js index 56ac7e8f4..436beecec 100644 --- a/src/actions/navigation.js +++ b/src/actions/navigation.js @@ -70,7 +70,7 @@ export const changePage = ({ } /* the path (dataset) remains the same... but the state may be modulated by the query */ - const newState = createStateFromQueryOrJSONs({oldState, query}); + const newState = createStateFromQueryOrJSONs({oldState, query, dispatch}); dispatch({ type: URL_QUERY_CHANGE_WITH_COMPUTED_STATE, ...newState, @@ -92,7 +92,7 @@ export const goTo404 = (errorMessage) => ({ export const EXPERIMENTAL_showMainDisplayMarkdown = ({query, queryToDisplay}) => (dispatch, getState) => { - const newState = createStateFromQueryOrJSONs({oldState: getState(), query}); + const newState = createStateFromQueryOrJSONs({oldState: getState(), query, dispatch}); newState.controls.panelsToDisplay = ["EXPERIMENTAL_MainDisplayMarkdown"]; dispatch({ type: URL_QUERY_CHANGE_WITH_COMPUTED_STATE, From bb27a5114c0c4f71c3daafa4b8a9cbb0fedce0f5 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Wed, 5 Feb 2020 09:21:08 +1300 Subject: [PATCH 19/22] Silently remove any `?clade=root` URL query 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. --- src/actions/recomputeReduxState.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/actions/recomputeReduxState.js b/src/actions/recomputeReduxState.js index 462d1c4cd..c743fe497 100644 --- a/src/actions/recomputeReduxState.js +++ b/src/actions/recomputeReduxState.js @@ -677,7 +677,9 @@ export const createStateFromQueryOrJSONs = ({ /* parse the query.label / query.clade */ if (query.clade) { - if (!query.label) query.label = `clade:${query.clade}`; + if (!query.label && query.clade !== "root") { + query.label = `clade:${query.clade}`; + } delete query.clade; } if (query.label) { From 943b566150ba1fed9d5a8eca1c2b204a0bd87384 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Wed, 5 Feb 2020 10:06:34 +1300 Subject: [PATCH 20/22] bugfix affecting tree zoom in narratives 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. --- src/actions/tree.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/actions/tree.js b/src/actions/tree.js index 455e0a218..83cac6158 100644 --- a/src/actions/tree.js +++ b/src/actions/tree.js @@ -21,7 +21,7 @@ export const applyInViewNodesToTree = (idx, tree) => { } else { applyToChildren(tree.nodes[validIdxRoot].shell, (d) => {d.inView = true;}); } - } else if (idx !== tree.idxOfInViewRootNode) { /* if shell isn't set yet - have set clade in URL */ + } else { tree.nodes.forEach((d) => { d.inView = false; }); From 9ef62eb153bcf5338783ba60072f34187de6e3a2 Mon Sep 17 00:00:00 2001 From: james hadfield Date: Wed, 5 Feb 2020 10:08:16 +1300 Subject: [PATCH 21/22] remove narrative console warnings about grid settings --- src/components/main/utils.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/components/main/utils.js b/src/components/main/utils.js index 3afb16666..03aee5092 100644 --- a/src/components/main/utils.js +++ b/src/components/main/utils.js @@ -22,11 +22,9 @@ export const calcPanelDims = (grid, panels, narrativeIsDisplayed, availableWidth } /* widths */ if (panels.includes("map") && panels.includes("tree") && !grid) { - console.warn("narrative mode specified full display but we have both map & tree"); bigWidthFraction = 0.5; } if (grid && (!panels.includes("map") || !panels.includes("tree"))) { - console.warn("narrative mode specified grid display but we are not showing both map & tree"); bigWidthFraction = 1; } } From 8397e4030ec5e8d245b1110e2b754ae410dd2d0c Mon Sep 17 00:00:00 2001 From: james hadfield Date: Wed, 5 Feb 2020 13:22:17 +1300 Subject: [PATCH 22/22] add `label` URL query to documentation --- docs-src/docs/advanced-functionality/view-settings.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs-src/docs/advanced-functionality/view-settings.md b/docs-src/docs/advanced-functionality/view-settings.md index 62aa5aa7b..6e53b2d0e 100644 --- a/docs-src/docs/advanced-functionality/view-settings.md +++ b/docs-src/docs/advanced-functionality/view-settings.md @@ -68,7 +68,8 @@ All URL queries modify the view away from the default settings -- if you change | `n` | Narrative page number | `n=1` goes to the first page | | `s` | Selected strain | `s=1_0199_PF` | | `branchLabel` | Branch labels to display | `branchLabel=aa` | -| `clade` | Labeled clade that tree is zoomed to | `clade=B3` (numeric values are buggy) | +| `label` | Labeled branch that tree is zoomed to | `label=clade:B3`, `label=lineage:relapse` | +| `clade` | _DEPRECATED_ Labeled clade that tree is zoomed to | `clade=B3` should now become `label=clade:B3` | **See this in action:**