Tree component refactoring #1863

victorlin · 2024-10-08T00:14:13Z

Description of proposed changes

Changes made while prepping for #1373. These can be considered independently from that PR. See individual commit messages.

Checklist

Checks pass
~~If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR~~ no functional changes
~~(to be done by a Nextstrain team member) Create preview PRs on downstream repositories.~~ no functional changes

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.

jameshadfield · 2024-10-09T00:46:17Z

src/components/tree/tree.js

  }
+
+  clearSelectedNode = () => {
+    callbacks.clearSelectedNode.bind(this);


[reminder to me] - check this change, I remember binding callbacks in the constructor was "the right way" way back when I was writing this code.

I see:

auspice/src/components/map/map.js

Lines 84 to 85 in 3cd0c1e

// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md#es6-classes

this.fitMapBoundsToData = this.fitMapBoundsToData.bind(this);

I've replaced 1eedee4 with a1aec38.

jameshadfield · 2024-10-09T00:46:48Z

@genehack could you take a look at the types added / used here if you have a chance? 🙏

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.

This allows type checking on the new entries.

This does not provide much benefit as-is, but should facilitate future usage of TypeScript within the component.

The commented out usage was removed in "Excise react-svg-pan-zoom" (56fbb8a).

Keep consistent with other parts of the file.

Only the clearSelectedNode binding needs to be kept in the constructor.

genehack · 2024-10-09T23:39:03Z

src/components/tree/tree.js

+    const anyTreeZoomed = this.props.tree.idxOfInViewRootNode !== 0 ||
      this.props.treeToo.idxOfInViewRootNode !== 0;

+    const activeResetTreeButton = anyTreeZoomed;


if you're gonna make these two variables the same thing why not just go the one extra step and rename treeIsZoomed directly to activeResetTreeButton, and then update the ternaries below to also use activeResetTreeButton? Seems less confusing in the longer run.

Good point, done: 4d8a2e0

I chose to go the other way around and drop activeResetTreeButton in favor of anyTreeZoomed to convey the condition rather than end state.

genehack · 2024-10-09T23:40:25Z

src/reducers/controls.ts

+interface Defaults {
+  distanceMeasure: string
+  layout: Layout
+  geoResolution: string
+  filters: Record<string, any>
+  filtersInFooter: string[]
+  colorBy: string
+  selectedBranchLabel: string
+  tipLabelKey: typeof strainSymbol
+  showTransmissionLines: boolean
+  sidebarOpen?: boolean
+}


I'd put this up around L15, so it's defined before being used in the (current) L18 -- was briefly confused where Defaults was even coming from.

Done:

auspice/src/reducers/controls.ts

Lines 15 to 32 in 4658803

type Layout = "rect" | "radial" | "unrooted" | "clock" | "scatter"

interface Defaults {

distanceMeasure: string

layout: Layout

geoResolution: string

filters: Record<string, any>

filtersInFooter: string[]

colorBy: string

selectedBranchLabel: string

tipLabelKey: typeof strainSymbol

showTransmissionLines: boolean

sidebarOpen?: boolean

}

export interface BasicControlsState {

defaults: Defaults

layout: Layout

genehack · 2024-10-09T23:45:19Z

src/reducers/controls.ts

+  filtersInFooter: string[]
+  colorBy: string
+  selectedBranchLabel: string
+  tipLabelKey: typeof strainSymbol


what's the advantage of doing this versus adding a proper type in ../utils/globals and exporting it from there?

I didn't want to dig a deeper hole and convert src/utils/globals.js to TypeScript. Even if I were to do that, I'm not sure that adding a proper type would be useful in this case. In globals.ts it would simply be:

- export const strainSymbol = Symbol('strain'); + export const strainSymbol: unique symbol = Symbol('strain');

and the same typeof strainSymbol would still be used here.

genehack · 2024-10-09T23:50:46Z

src/reducers/narrative.ts

+  state: NarrativeState = defaultState,
+  action: AnyAction,


can you provide a default value to a positional param if there's a subsequent positional param without a default value? or do you need to switch the order here, or convert this to being passed an object?

Good point – ESLint and TypeScript clearly had no complaints about this so I searched to confirm – it is allowed in JavaScript. From MDN docs on default parameters:

Parameters are still set left-to-right, overwriting default parameters even if there are later parameters without defaults.

function f(x = 1, y) { return [x, y]; } f(); // [1, undefined] f(2); // [2, undefined]

I don't think it's a good pattern, but it is the pattern used for other files in src/reducers/ so I will keep it as-is.

- `treeIsZoomed` is ambiguous since the condition considers two trees. Rename to `anyTreeZoomed`. - `activeResetTreeButton` has the same condition as `anyTreeZoomed`. Reuse the existing variable.

Partial<ControlsState> previously worked but has the side effect of marking all keys as optional, which is not accurate and prone to cause future type errors.

victorlin self-assigned this Oct 8, 2024

nextstrain-bot temporarily deployed to auspice-victorlin-prep--fjlw4s October 8, 2024 00:14 Inactive

victorlin mentioned this pull request Oct 8, 2024

tree: Add toggle to focus on selected #1373

Open

13 tasks

victorlin temporarily deployed to auspice-victorlin-prep--fjlw4s October 8, 2024 00:49 Inactive

victorlin marked this pull request as ready for review October 8, 2024 00:52

Add type to top-level state

27c1c27

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.

victorlin force-pushed the victorlin/prep-dynamic-yvalues branch from 9ecca22 to 8280aaa Compare October 8, 2024 18:00

victorlin temporarily deployed to auspice-victorlin-prep--fjlw4s October 8, 2024 18:00 Inactive

victorlin force-pushed the victorlin/prep-dynamic-yvalues branch from 8280aaa to 33277b5 Compare October 8, 2024 23:28

victorlin temporarily deployed to auspice-victorlin-prep--fjlw4s October 8, 2024 23:28 Inactive

jameshadfield reviewed Oct 9, 2024

View reviewed changes

victorlin temporarily deployed to auspice-victorlin-prep--fjlw4s October 9, 2024 17:22 Inactive

victorlin added 6 commits October 9, 2024 14:39

Use spread operator instead of Object.assign

4822250

This allows type checking on the new entries.

Add state type to Tree component

ff6caa6

This does not provide much benefit as-is, but should facilitate future usage of TypeScript within the component.

Remove unused comment

91fc8ef

The commented out usage was removed in "Excise react-svg-pan-zoom" (56fbb8a).

Add whitespace

2a45510

Keep consistent with other parts of the file.

Move non-binding functions out of constructor

a1aec38

Only the clearSelectedNode binding needs to be kept in the constructor.

victorlin force-pushed the victorlin/prep-dynamic-yvalues branch from 6d65167 to ceec881 Compare October 9, 2024 21:40

victorlin temporarily deployed to auspice-victorlin-prep--fjlw4s October 9, 2024 21:41 Inactive

genehack reviewed Oct 9, 2024

View reviewed changes

victorlin added 3 commits October 11, 2024 10:02

Clarify button active conditions

4d8a2e0

- `treeIsZoomed` is ambiguous since the condition considers two trees. Rename to `anyTreeZoomed`. - `activeResetTreeButton` has the same condition as `anyTreeZoomed`. Reuse the existing variable.

Add type for state.controls.defaults

b1a26c6

Partial<ControlsState> previously worked but has the side effect of marking all keys as optional, which is not accurate and prone to cause future type errors.

Add type for state.controls.layout

4658803

victorlin force-pushed the victorlin/prep-dynamic-yvalues branch from ceec881 to 4658803 Compare October 11, 2024 17:37

victorlin deployed to auspice-victorlin-prep--fjlw4s October 11, 2024 17:38 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree component refactoring #1863

Tree component refactoring #1863

victorlin commented Oct 8, 2024 •

edited

Loading

jameshadfield Oct 9, 2024 •

edited

Loading

victorlin Oct 9, 2024

jameshadfield commented Oct 9, 2024

genehack Oct 9, 2024

victorlin Oct 11, 2024

genehack Oct 9, 2024

victorlin Oct 11, 2024

genehack Oct 9, 2024

victorlin Oct 11, 2024

genehack Oct 9, 2024

victorlin Oct 11, 2024

victorlin Oct 11, 2024

	// https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md#es6-classes
	this.fitMapBoundsToData = this.fitMapBoundsToData.bind(this);

	type Layout = "rect" \| "radial" \| "unrooted" \| "clock" \| "scatter"

	interface Defaults {
	distanceMeasure: string
	layout: Layout
	geoResolution: string
	filters: Record<string, any>
	filtersInFooter: string[]
	colorBy: string
	selectedBranchLabel: string
	tipLabelKey: typeof strainSymbol
	showTransmissionLines: boolean
	sidebarOpen?: boolean
	}

	export interface BasicControlsState {
	defaults: Defaults
	layout: Layout

Tree component refactoring #1863

Are you sure you want to change the base?

Tree component refactoring #1863

Conversation

victorlin commented Oct 8, 2024 • edited Loading

Description of proposed changes

Checklist

jameshadfield Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameshadfield commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorlin commented Oct 8, 2024 •

edited

Loading

jameshadfield Oct 9, 2024 •

edited

Loading