From 27c1c27fe53600c968e4c09c776c88f541c7008a Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Mon, 7 Oct 2024 14:40:29 -0700 Subject: [PATCH 01/10] Add type to top-level state Using ReturnType on all sub-states except for controls (which already has a type to be used directly) and narrative (the compiler was not able to resolve types between return values and default state). The next commit will fix the narrative type. --- src/reducers/{index.js => index.ts} | 19 +++++++++++++++++-- src/store.ts | 1 + 2 files changed, 18 insertions(+), 2 deletions(-) rename src/reducers/{index.js => index.ts} (51%) diff --git a/src/reducers/index.js b/src/reducers/index.ts similarity index 51% rename from src/reducers/index.js rename to src/reducers/index.ts index 70a568a63..00accb5b8 100644 --- a/src/reducers/index.js +++ b/src/reducers/index.ts @@ -3,7 +3,7 @@ import metadata from "./metadata"; import tree from "./tree"; import frequencies from "./frequencies"; import entropy from "./entropy"; -import controls from "./controls"; +import controls, { ControlsState } from "./controls"; import browserDimensions from "./browserDimensions"; import notifications from "./notifications"; import narrative from "./narrative"; @@ -12,7 +12,22 @@ import general from "./general"; import jsonCache from "./jsonCache"; import measurements from "./measurements"; -const rootReducer = combineReducers({ +interface RootState { + metadata: ReturnType + tree: ReturnType + frequencies: ReturnType + controls: ControlsState + entropy: ReturnType + browserDimensions: ReturnType + notifications: ReturnType + narrative: any + treeToo: ReturnType + general: ReturnType + jsonCache: ReturnType + measurements: ReturnType +} + +const rootReducer = combineReducers({ metadata, tree, frequencies, diff --git a/src/store.ts b/src/store.ts index 829d94d98..4943b23ae 100644 --- a/src/store.ts +++ b/src/store.ts @@ -48,6 +48,7 @@ if (process.env.NODE_ENV !== 'production' && module.hot) { } // Infer types from the store. +// This is more clearly defined in src/reducers/index.ts but exported here. export type RootState = ReturnType export type AppDispatch = typeof store.dispatch From 3048a4dad4b7f119feb79328804de7d91650ec3c Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Mon, 7 Oct 2024 14:47:56 -0700 Subject: [PATCH 02/10] Add types to narrative state This was necessary to properly type the root state. Note: AnyAction is used for `action` which is only slightly better than any. Ideally, each action type would come with its own TypeScript interface but there is no good way to tightly couple those. I think the right improvement here is to use Redux's createSlice but that is beyond the scope of adding types. --- src/reducers/index.ts | 4 +-- src/reducers/{narrative.js => narrative.ts} | 37 ++++++++++++++++++--- 2 files changed, 34 insertions(+), 7 deletions(-) rename src/reducers/{narrative.js => narrative.ts} (66%) diff --git a/src/reducers/index.ts b/src/reducers/index.ts index 00accb5b8..c6fcf7c7b 100644 --- a/src/reducers/index.ts +++ b/src/reducers/index.ts @@ -6,7 +6,7 @@ import entropy from "./entropy"; import controls, { ControlsState } from "./controls"; import browserDimensions from "./browserDimensions"; import notifications from "./notifications"; -import narrative from "./narrative"; +import narrative, { NarrativeState } from "./narrative"; import treeToo from "./treeToo"; import general from "./general"; import jsonCache from "./jsonCache"; @@ -20,7 +20,7 @@ interface RootState { entropy: ReturnType browserDimensions: ReturnType notifications: ReturnType - narrative: any + narrative: NarrativeState treeToo: ReturnType general: ReturnType jsonCache: ReturnType diff --git a/src/reducers/narrative.js b/src/reducers/narrative.ts similarity index 66% rename from src/reducers/narrative.js rename to src/reducers/narrative.ts index 935f81f29..f23cc5c94 100644 --- a/src/reducers/narrative.js +++ b/src/reducers/narrative.ts @@ -1,13 +1,40 @@ import * as types from "../actions/types"; +import { AnyAction } from 'redux'; -const narrative = (state = { +export interface NarrativeState { + loaded: boolean + /** + * array of paragraphs (aka blocks) + */ + blocks: { __html: string }[] | null + + /** + * which block is currently "in view" + */ + blockIdx?: number + + /** + * the pathname of the _narrative_ + */ + pathname?: string + + display: boolean + title?: string +} + +const defaultState: NarrativeState = { loaded: false, - blocks: null, /* array of paragraphs (aka blocks) */ - blockIdx: undefined, /* which block is currently "in view" */ - pathname: undefined, /* the pathname of the _narrative_ */ + blocks: null, + blockIdx: undefined, + pathname: undefined, display: false, title: undefined -}, action) => { +}; + +const narrative = ( + state: NarrativeState = defaultState, + action: AnyAction, +): NarrativeState => { switch (action.type) { case types.DATA_INVALID: return Object.assign({}, state, { From 4822250ebc18993a0d20e4d844da5d2ac1b39bbb Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Wed, 9 Oct 2024 14:24:29 -0700 Subject: [PATCH 03/10] Use spread operator instead of Object.assign This allows type checking on the new entries. --- src/reducers/narrative.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/reducers/narrative.ts b/src/reducers/narrative.ts index f23cc5c94..ad0459e89 100644 --- a/src/reducers/narrative.ts +++ b/src/reducers/narrative.ts @@ -37,10 +37,11 @@ const narrative = ( ): NarrativeState => { switch (action.type) { case types.DATA_INVALID: - return Object.assign({}, state, { + return { + ...state, loaded: false, - display: false - }); + display: false, + }; case types.CLEAN_START: if (action.narrative) { const blocks = action.narrative; @@ -56,12 +57,18 @@ const narrative = ( return state; case types.URL_QUERY_CHANGE_WITH_COMPUTED_STATE: if (Object.prototype.hasOwnProperty.call(action.query, "n")) { - return Object.assign({}, state, {blockIdx: action.query.n}); + return { + ...state, + blockIdx: action.query.n, + }; } return state; case types.TOGGLE_NARRATIVE: if (state.loaded) { - return Object.assign({}, state, {display: action.narrativeOn}); + return { + ...state, + display: action.narrativeOn, + }; } console.warn("Attempted to toggle narrative that was not loaded"); return state; From ff6caa62be57a1824f22b31c16ccbb98b991f8f1 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Mon, 7 Oct 2024 14:49:02 -0700 Subject: [PATCH 04/10] Add state type to Tree component This does not provide much benefit as-is, but should facilitate future usage of TypeScript within the component. --- src/components/tree/{index.js => index.ts} | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) rename src/components/tree/{index.js => index.ts} (93%) diff --git a/src/components/tree/index.js b/src/components/tree/index.ts similarity index 93% rename from src/components/tree/index.js rename to src/components/tree/index.ts index ded81bef0..b7931c96b 100644 --- a/src/components/tree/index.js +++ b/src/components/tree/index.ts @@ -1,7 +1,8 @@ import { connect } from "react-redux"; import UnconnectedTree from "./tree"; +import { RootState } from "../../store"; -const Tree = connect((state) => ({ +const Tree = connect((state: RootState) => ({ tree: state.tree, treeToo: state.treeToo, selectedNode: state.controls.selectedNode, From 91fc8ef8502b05ca6465b364f90c0e40af98c8c1 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Fri, 20 Sep 2024 15:32:20 -0700 Subject: [PATCH 05/10] Remove unused comment The commented out usage was removed in "Excise react-svg-pan-zoom" (56fbb8a1). --- src/components/tree/tree.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/tree/tree.js b/src/components/tree/tree.js index 6589d577c..92b498098 100644 --- a/src/components/tree/tree.js +++ b/src/components/tree/tree.js @@ -37,7 +37,6 @@ class Tree extends React.Component { }; /* bind callbacks */ this.clearSelectedNode = callbacks.clearSelectedNode.bind(this); - // this.handleIconClickHOF = callbacks.handleIconClickHOF.bind(this); this.redrawTree = () => { this.props.dispatch(updateVisibleTipsAndBranchThicknesses({ root: [0, 0] From 2a455107a040c04291387f0adaac74d6264dff54 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Fri, 20 Sep 2024 15:53:44 -0700 Subject: [PATCH 06/10] Add whitespace Keep consistent with other parts of the file. --- src/components/tree/tree.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/tree/tree.js b/src/components/tree/tree.js index 92b498098..bba1cb0f6 100644 --- a/src/components/tree/tree.js +++ b/src/components/tree/tree.js @@ -49,6 +49,7 @@ class Tree extends React.Component { } }; } + setUpAndRenderTreeToo(props, newState) { /* this.setState(newState) will be run sometime after this returns */ /* modifies newState in place */ @@ -58,6 +59,7 @@ class Tree extends React.Component { } renderTree(this, false, newState.treeToo, props); } + componentDidMount() { document.addEventListener('keyup', this.handlekeydownEvent); if (this.props.tree.loaded) { @@ -71,6 +73,7 @@ class Tree extends React.Component { this.setState(newState); /* this will trigger an unnecessary CDU :( */ } } + componentDidUpdate(prevProps) { let newState = {}; let rightTreeUpdated = false; From a1aec383711667c0d4c26292445eb0a4f07b2f32 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Fri, 20 Sep 2024 15:55:02 -0700 Subject: [PATCH 07/10] Move non-binding functions out of constructor Only the clearSelectedNode binding needs to be kept in the constructor. --- src/components/tree/tree.js | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/components/tree/tree.js b/src/components/tree/tree.js index bba1cb0f6..264523d67 100644 --- a/src/components/tree/tree.js +++ b/src/components/tree/tree.js @@ -35,19 +35,22 @@ class Tree extends React.Component { tree: null, treeToo: null }; + /* bind callbacks */ this.clearSelectedNode = callbacks.clearSelectedNode.bind(this); - this.redrawTree = () => { - this.props.dispatch(updateVisibleTipsAndBranchThicknesses({ - root: [0, 0] - })); - }; - /* pressing the escape key should dismiss an info modal (if one exists) */ - this.handlekeydownEvent = (event) => { - if (event.key==="Escape" && this.props.selectedNode) { - this.clearSelectedNode(this.props.selectedNode); - } - }; + } + + redrawTree = () => { + this.props.dispatch(updateVisibleTipsAndBranchThicknesses({ + root: [0, 0] + })); + } + + /* pressing the escape key should dismiss an info modal (if one exists) */ + handlekeydownEvent = (event) => { + if (event.key==="Escape" && this.props.selectedNode) { + this.clearSelectedNode(this.props.selectedNode); + } } setUpAndRenderTreeToo(props, newState) { From 4d8a2e04989bcb148ff21a0e12f4b0801ecbe3d6 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Fri, 4 Oct 2024 14:51:58 -0700 Subject: [PATCH 08/10] Clarify button active conditions - `treeIsZoomed` is ambiguous since the condition considers two trees. Rename to `anyTreeZoomed`. - `activeResetTreeButton` has the same condition as `anyTreeZoomed`. Reuse the existing variable. --- src/components/tree/tree.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/components/tree/tree.js b/src/components/tree/tree.js index 264523d67..adee6cf43 100644 --- a/src/components/tree/tree.js +++ b/src/components/tree/tree.js @@ -115,16 +115,13 @@ class Tree extends React.Component { } getStyles = () => { - const activeResetTreeButton = this.props.tree.idxOfInViewRootNode !== 0 || - this.props.treeToo.idxOfInViewRootNode !== 0; - const filteredTree = !!this.props.tree.idxOfFilteredRoot && this.props.tree.idxOfInViewRootNode !== this.props.tree.idxOfFilteredRoot; const filteredTreeToo = !!this.props.treeToo.idxOfFilteredRoot && this.props.treeToo.idxOfInViewRootNode !== this.props.treeToo.idxOfFilteredRoot; const activeZoomButton = filteredTree || filteredTreeToo; - const treeIsZoomed = this.props.tree.idxOfInViewRootNode !== 0 || + const anyTreeZoomed = this.props.tree.idxOfInViewRootNode !== 0 || this.props.treeToo.idxOfInViewRootNode !== 0; return { @@ -138,8 +135,8 @@ class Tree extends React.Component { zIndex: 100, display: "inline-block", marginLeft: 4, - cursor: activeResetTreeButton ? "pointer" : "auto", - color: activeResetTreeButton ? darkGrey : lightGrey + cursor: anyTreeZoomed ? "pointer" : "auto", + color: anyTreeZoomed ? darkGrey : lightGrey }, zoomToSelectedButton: { zIndex: 100, @@ -151,9 +148,9 @@ class Tree extends React.Component { zoomOutButton: { zIndex: 100, display: "inline-block", - cursor: treeIsZoomed ? "pointer" : "auto", - color: treeIsZoomed ? darkGrey : lightGrey, - pointerEvents: treeIsZoomed ? "auto" : "none", + cursor: anyTreeZoomed ? "pointer" : "auto", + color: anyTreeZoomed ? darkGrey : lightGrey, + pointerEvents: anyTreeZoomed ? "auto" : "none", marginRight: "4px" } }; From b1a26c625b8e2d15e57adaf4d356755669959516 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Mon, 7 Oct 2024 17:43:58 -0700 Subject: [PATCH 09/10] Add type for state.controls.defaults Partial previously worked but has the side effect of marking all keys as optional, which is not accurate and prone to cause future type errors. --- src/reducers/controls.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/reducers/controls.ts b/src/reducers/controls.ts index 6682271a0..7428750ea 100644 --- a/src/reducers/controls.ts +++ b/src/reducers/controls.ts @@ -12,7 +12,21 @@ import { calcBrowserDimensionsInitialState } from "./browserDimensions"; import { doesColorByHaveConfidence } from "../actions/recomputeReduxState"; import { hasMultipleGridPanels } from "../actions/panelDisplay"; +interface Defaults { + distanceMeasure: string + layout: string + geoResolution: string + filters: Record + filtersInFooter: string[] + colorBy: string + selectedBranchLabel: string + tipLabelKey: typeof strainSymbol + showTransmissionLines: boolean + sidebarOpen?: boolean +} + export interface BasicControlsState { + defaults: Defaults panelsAvailable: string[] panelsToDisplay: string[] showTreeToo: boolean @@ -40,7 +54,7 @@ export interface ControlsState extends BasicControlsState, MeasurementsControlSt at any time, e.g. if we want to revert things (e.g. on dataset change) */ export const getDefaultControlsState = () => { - const defaults: Partial = { + const defaults: Defaults = { distanceMeasure: defaultDistanceMeasure, layout: defaultLayout, geoResolution: defaultGeoResolution, From 46588036488a0132423f7eb01815aa658c1686e3 Mon Sep 17 00:00:00 2001 From: Victor Lin <13424970+victorlin@users.noreply.github.com> Date: Tue, 8 Oct 2024 16:25:13 -0700 Subject: [PATCH 10/10] Add type for state.controls.layout --- src/reducers/controls.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/reducers/controls.ts b/src/reducers/controls.ts index 7428750ea..2c2a5ae2a 100644 --- a/src/reducers/controls.ts +++ b/src/reducers/controls.ts @@ -12,9 +12,11 @@ import { calcBrowserDimensionsInitialState } from "./browserDimensions"; import { doesColorByHaveConfidence } from "../actions/recomputeReduxState"; import { hasMultipleGridPanels } from "../actions/panelDisplay"; +type Layout = "rect" | "radial" | "unrooted" | "clock" | "scatter" + interface Defaults { distanceMeasure: string - layout: string + layout: Layout geoResolution: string filters: Record filtersInFooter: string[] @@ -27,6 +29,7 @@ interface Defaults { export interface BasicControlsState { defaults: Defaults + layout: Layout panelsAvailable: string[] panelsToDisplay: string[] showTreeToo: boolean