Color tree by measurements #1924
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,10 +65,18 @@ interface Query extends MeasurementsQuery { | |
| [key: string]: string | string[] | ||
| } | ||
|
|
||
|
|
||
| const hasMeasurementColorAttr = "_hasMeasurementColor"; | ||
| const hasMeasurementColorValue = "true"; | ||
| interface MeasurementsNodeAttrs { | ||
| [strain: string]: { | ||
| [key: string]: { | ||
| // number for the average measurements value | ||
| // 'true' for the presence of measurements coloring | ||
| value: number | typeof hasMeasurementColorValue | ||
| } | ||
| [hasMeasurementColorAttr]: { | ||
| value: typeof hasMeasurementColorValue | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -666,6 +674,9 @@ function createMeasurementsColoringData( | |
| nodeAttrs[strain] = { | ||
| [measurementColorBy]: { | ||
| value: averageMeasurementValue | ||
| }, | ||
| [hasMeasurementColorAttr]: { | ||
| value: hasMeasurementColorValue | ||
| } | ||
| }; | ||
| } | ||
|
|
@@ -687,6 +698,10 @@ function createMeasurementsColoringData( | |
| title: `Measurements (${groupingValue})`, | ||
| type: "continuous", | ||
| scale: measurementsColorScale, | ||
| }, | ||
| [hasMeasurementColorAttr]: { | ||
| title: `Has measurements for ${groupingValue}`, | ||
| type: "boolean", | ||
| } | ||
| } | ||
| }; | ||
|
|
@@ -726,7 +741,7 @@ function updateMeasurementsColorData( | |
| } | ||
| dispatch({ | ||
| type: REMOVE_METADATA, | ||
| nodeAttrsToRemove: [hasMeasurementColorAttr, encodeMeasurementColorBy(oldColorGrouping)], | ||
| }) | ||
| } | ||
| /* If there is a valid new color grouping, then add the measurement metadata and coloring */ | ||
|
|
@@ -748,7 +763,7 @@ export const applyMeasurementsColorBy = ( | |
| */ | ||
| batch(() => { | ||
|
There was a problem hiding this comment. I didn't know about There was a problem hiding this comment.
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. There was a problem hiding this comment. 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: [hasMeasurementColorAttr, encodeMeasurementColorBy(controls.measurementsColorGrouping)]}); | ||
| } | ||
| if (controls.measurementsColorGrouping !== groupingValue) { | ||
| dispatch({type: CHANGE_MEASUREMENTS_COLOR_GROUPING, controls:{measurementsColorGrouping: groupingValue}}); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably beyond this PR, but this makes me think about some improvements to the filter badges - they haven't seen any development since they were first introduced and filtering has become a much bigger part of workflows since then.
It'd be nice to show the title not the key when hovering, and even nicer if we could somehow render this in the badge itself, as a badge simply showing "true" isn't very intuitive. We already do both of these in the sidebar
Would this (the
_hasMeasurementsColorfilter) be better as part of the separate measurements filtering section?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, these would be nice, but it might overcrowd the badges with the longer titles. We can revisit the design of the filter badges separately.
I think it makes sense to keep the
_hasMeasurementsColorfilter in the overall filters since it is filtering the tree node attributes. The measurements filtering section is specific to filtering measurements records in the measurements panel.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, this is clarifying. Thanks!