Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Color tree by measurements #1924

Merged
merged 35 commits into from
Jan 21, 2025
Merged

Color tree by measurements #1924

Changes from 1 commit
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
d90c503
MeasurementsControlState: Remove unnecessary trailing commas
joverlee521 Dec 18, 2024
6c2e53b
measurements: Fix bug for numeric legend values in ordering
joverlee521 Dec 12, 2024
15dabdf
Add color tree by measurements feature
joverlee521 Dec 10, 2024
14a5650
ADD_EXTRA_METADATA: only use non-continuous colorings as filter traits
joverlee521 Dec 19, 2024
1391b9b
applyMeasurementsColorBy: Use multiple anchors for color scale
joverlee521 Dec 11, 2024
acf11a4
measurements: Refactor getting matching filters into separate function
joverlee521 Dec 12, 2024
a4b4d9c
applyMeasurementsColorBy: Only include measurements that pass filters
joverlee521 Dec 13, 2024
69853e2
measurements: Save color grouping value as a separate state
joverlee521 Dec 18, 2024
8b2773e
applyMeasurementsColorBy: remove old measurements coloring data
joverlee521 Dec 20, 2024
74eac4a
applyMeasurementsColorBy: batch dispatch actions
joverlee521 Dec 20, 2024
c0b7e91
Refactor `applyMeasurementsColorBy` into small functions
joverlee521 Dec 19, 2024
e1c74a4
Update measurements coloring when changing collection
joverlee521 Dec 24, 2024
9a5a183
Update measurements coloring when changing group by
joverlee521 Dec 24, 2024
3c35ed5
Update measurement coloring when updating measurements filters
joverlee521 Dec 21, 2024
c2db2bb
Update type `ColoringInfo`
joverlee521 Dec 24, 2024
17d411c
Refactor `addMeasurementsColorData` into smaller function
joverlee521 Dec 24, 2024
ade2c20
Support measurements coloring URL param `c=m-<grouping_value>`
joverlee521 Dec 24, 2024
440cddd
Add crosshair in measurements panel y-axis for coloring
joverlee521 Dec 30, 2024
ff28517
measurements: Allow click on grouping when current colorby is different
joverlee521 Dec 31, 2024
795efc7
Remove extra whitespace
joverlee521 Jan 2, 2025
9a4c328
tree: Add crosshair on tip matching measurements color grouping
joverlee521 Jan 2, 2025
5d51e9c
tree: re-draw measurements crosshair when modifying SVG
joverlee521 Jan 3, 2025
0dbc4e3
tree: update measurements crosshair when changing colorBy
joverlee521 Jan 3, 2025
fb3eba0
narratives: Ensure tree coloring is calculated for measurements colors
joverlee521 Jan 3, 2025
a2cfe54
narratives: Ensure old measurements coloring data is removed
joverlee521 Jan 3, 2025
bb44c2a
modifySVGInStages: remove crosshair in step 1
joverlee521 Jan 6, 2025
82e6f6d
Add comments for measurementColoringPrefix
joverlee521 Jan 7, 2025
9f6b032
Add tooltip to hint at color by measurements feature
joverlee521 Jan 7, 2025
27cdbb0
Click on measurements grouping to toggle back to previous coloring
joverlee521 Jan 8, 2025
e52f48d
Update type ColoringInfo: make `scale` optional
joverlee521 Jan 9, 2025
972ed91
Add `_hasMeasurementColor` attr and coloring for measurements coloring
joverlee521 Jan 9, 2025
e586a6c
Filter to colored tips when applying measurements coloring
joverlee521 Jan 9, 2025
921161c
measurements: Use `String` instead of `toString`
joverlee521 Jan 21, 2025
aead1cf
removeNodeAttrs: drop unnecessary conditional
joverlee521 Jan 21, 2025
daccd78
Update changelog
joverlee521 Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions src/actions/measurements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { batch } from "react-redux";
import { quantile } from "d3-array";
import { cloneDeep } from "lodash";
import { AppDispatch, ThunkFunction } from "../store";
Expand Down Expand Up @@ -611,14 +612,23 @@ export const applyMeasurementsColorBy = (
}
}

if (controls.measurementsColorGrouping !== undefined) {
dispatch({type: REMOVE_METADATA, nodeAttrsToRemove: [encodeMeasurementColorBy(controls.measurementsColorGrouping)]});
}
if (controls.measurementsColorGrouping !== groupingValue) {
dispatch({type: CHANGE_MEASUREMENTS_COLOR_GROUPING, controls:{measurementsColorGrouping: groupingValue}});
}
dispatch({type: ADD_EXTRA_METADATA, newNodeAttrs, newColorings});
dispatch(changeColorBy(measurementColorBy));
/**
* Batching all dispatch actions together to prevent multiple renders
* This is also _required_ to prevent error in calcColorScale during extra renders:
* 1. REMOVE_METADATA removes current measurements coloring from metadata.colorings
* 2. This triggers the componentDidUpdate in controls/color-by, which dispatches changeColorBy.
* 3. calcColorScale throws error because the current coloring is no longer valid as it was removed by REMOVE_METADATA in step 1.
*/
batch(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about batch - was a good opportunity to catch up with some of the many changes in the redux/react world. It seems that React 18 will do this automatically (react 18 upgrade PR: #1659). I wondered why you didn't just add the new node info, change colour, then delete the old data... however since the node.attrs key names are reused I can imagine this causes some undesirable side-effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered why you didn't just add the new node info, change colour, then delete the old data... however since the node.attrs key names are reused I can imagine this causes some undesirable side-effects?

Yeah, things can get complicated if the node.attrs key names are the same but the underlying values changed. A concrete example is changing measurements collections. They might have the same reference strain (which means they have the same node.attrs key), but completely different measurements so the data needs to be completely updated. It's cleaner to remove old data then apply the new data instead of trying to keep track of which nodes needs to be updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth changing what you've done (which includes a good docstring) but an alternative approach to consider for patterns like this if we use them again is to make "remove metadata" a thunk which checks that the attr being removed isn't currently used as a coloring, and if it is then thee thunk can do something about it.

if (controls.measurementsColorGrouping !== undefined) {
dispatch({type: REMOVE_METADATA, nodeAttrsToRemove: [encodeMeasurementColorBy(controls.measurementsColorGrouping)]});
}
if (controls.measurementsColorGrouping !== groupingValue) {
dispatch({type: CHANGE_MEASUREMENTS_COLOR_GROUPING, controls:{measurementsColorGrouping: groupingValue}});
}
dispatch({type: ADD_EXTRA_METADATA, newNodeAttrs, newColorings});
dispatch(changeColorBy(measurementColorBy));
})
}

const controlToQueryParamMap = {
Expand Down