Implement advanced map marker configuration menu #285

jernestmyers · 2023-06-07T14:14:33Z

Resolves #190

I'm most unsure still about how I'm handling the continuousMarkerPreview data. Not certain this is a great approach, or if the data is being displayed as we're expecting it to display. Aside from that, I have some concerns about code duplication in the different marker config menus but I fear that extracting that duplication would create a lot more code in MapAnalysis where things would get passed into a generic marker config menu component.

Then there are UX/UI decisions to be made.

how do/should we style the continuous var histograms (we know plotly components aren't the easiest layouts to configure!)
do we want values in the marker previews?
how should we handle/communicate value selections in the categorical value tables?
certainly many more!

…me code

adnauseum · 2023-06-09T19:59:57Z

packages/libs/eda/src/lib/map/analysis/MarkerConfiguration/BarPlotMarkerConfigurationMenu.tsx

 }

+// TODO: generalize this and PieMarkerConfigMenu into MarkerConfigurationMenu. Lots of code repitition...


https://blog.codinghorror.com/rule-of-three/

Perhaps I'll change it to a "maybe do" 😆

🙃
Just wanted to derp by the water ~~color~~ cooler 👋 . I've been painting recently... but with acrylics..

bobular · 2023-06-14T10:46:16Z

Reviewing this now.

#279 seems important as an "up next"?

bobular

I think the big nut to crack is the checkbox column heading behaviour in CategoricalMarkerConfigurationTable

At the moment it seems to behave erratically. With SCORE S. mansoni "occupation" it will create an endless loop (or at least for the number of items). If "Peasant" is unselected first, then the very last value "Transporter" gets selected.

With low cardinality variables it seems to deselect the last value only.

Some options:

somehow hack the Mesa/DataTable component to disable the heading checkbox (I looked already there's no easy prop to do this)
keep the heading checkbox but make the first click deselect all (also requires some hacking I think) - and when it is clicked again, don't show endless warnings.
gate off the value selection behind [:heavy_check_mark: apply] [:x: cancel] buttons, and leave the table implementation alone

I think something is a bit off with the handleSelection and handleDeselection handlers but @dmfalke probably understands quicker. They are called in a forEach which explains the multiple alerts.

  selectAllRows() {
    const { rows, options, eventHandlers } = this.props;
    const { isRowSelected } = options;
    const { onRowSelect, onMultipleRowSelect } = eventHandlers;
    const unselectedRows = rows.filter((row) => !isRowSelected(row));
    if (onMultipleRowSelect) return onMultipleRowSelect(unselectedRows);
    return unselectedRows.forEach(onRowSelect);
  }

from SelectionCell.jsx

Here, onRowSelect is the handleSelection handler we pass in.

bobular · 2023-06-14T12:26:10Z

Unless perhaps we can use the onMultipleRowSelect and onMultipleRowDeselect handlers?

However these are only applied to unselected and selected rows respectively. I think we'd need our putative toggleSelectAll handler to know about selected and unselected rows.

- apply deselectAll handler if indeterminate checkbox is clicked

jernestmyers · 2023-06-14T21:20:48Z

Unless perhaps we can use the onMultipleRowSelect and onMultipleRowDeselect handlers?

However these are only applied to unselected and selected rows respectively. I think we'd need our putative toggleSelectAll handler to know about selected and unselected rows.

The latest push makes use of onMultipleRowSelect and onMultipleRowDeselect. More specifically...

onMultipleRowSelect is currently selecting all values, which makes the legend and the markers look quite strange
onMultipleRowDeselect sets selectedValue to be only __UNSELECTED__, which renders as All other values in the legend; this handler is also now triggered when the indeterminate-state checkbox is clicked in the header.

Decisions that remain:
How to handle selecting too many values?

Should we add intermediate state that can be used to show the boxes as checked while we continue to aggregate any "excess" selections as __UNSELECTED__? We would still need to communicate to the use that, "Hey, we can only select X unique values. All others will be aggregated as All other values."
🤷

…elevant

…ible toggles

…prop

jernestmyers · 2023-06-28T20:50:20Z

packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx

@@ -272,28 +283,110 @@ function MapAnalysisImpl(props: ImplProps) {
    [markerConfigurations, setMarkerConfigurations]
  );

+  const filtersIncludingViewport = useMemo(() => {


NOTE: No logic/code has changed for filtersIncludingViewport but I had to move its initialization above allVisibleCategoricalValues.

bobular

This is looking and working great.

If you choose not to follow any of my suggestions please make tickets for them.

I'll come back laters to the io-ts query I made in appState.ts

packages/libs/eda/src/lib/map/analysis/hooks/standaloneMapMarkers.tsx

...es/libs/eda/src/lib/map/analysis/MarkerConfiguration/CategoricalMarkerConfigurationTable.tsx

bobular · 2023-06-29T11:29:50Z

packages/libs/eda/src/lib/map/analysis/utils/categoricalValues.ts

+    const unfilteredDistributionResponse =
+      await subsettingClient.getDistribution(
+        studyId,
+        overlayEntity.id,
+        overlayVariable.id,
+        {
+          valueSpec: 'count',
+          filters: [],
+        }
+      );


It's not urgent - could be a follow-up ticket - but I think these unfiltered values+counts should be cached somehow.

I can't think of a simple drop-in way to do this. My suggestion would involve a new hook called from MapAnalysis that generates unfilteredValues and pass this as yet another prop into getCategoricalValues. This isn't nice though. It unnecessarily exposes things that are rightly "hidden" here in this function.

I'm sure @dmfalke will have a wise suggestion.

I don't have any suggestions right now, but I will think about it some more.

packages/libs/eda/src/lib/map/analysis/utils/categoricalValues.ts

packages/libs/eda/src/lib/map/analysis/hooks/standaloneMapMarkers.tsx

bobular · 2023-06-29T12:09:09Z

packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx

+  const { markersData: previewMarkerData } = useStandaloneMapMarkers({
+    boundsZoomLevel: undefined,
+    geoConfig: geoConfig,
+    studyId,
+    filters,
+    markerType,
+    selectedOverlayVariable: activeMarkerConfiguration?.selectedVariable,
+    overlayConfig: activeOverlayConfig.value,
+    outputEntityId: outputEntity?.id,
+  });
+
+  const continuousMarkerPreview = useMemo(() => {
+    if (!previewMarkerData || !previewMarkerData.length) return;
+    const initialDataObject = previewMarkerData[0].data.map((data) => ({
+      label: data.label,
+      value: 0,
+      ...(data.color ? { color: data.color } : {}),
+    }));
+    const typedData =
+      markerType === 'pie'
+        ? ([...previewMarkerData] as DonutMarkerProps[])
+        : ([...previewMarkerData] as ChartMarkerProps[]);
+    const finalData = typedData.reduce(
+      (prevData, currData) =>
+        currData.data.map((data, index) => ({
+          label: data.label,
+          value: data.value + prevData[index].value,
+          ...('color' in prevData[index]
+            ? { color: prevData[index].color }
+            : 'color' in data
+            ? { color: data.color }
+            : {}),
+        })),
+      initialDataObject
+    );
+    if (markerType === 'pie') {
+      return (
+        <DonutMarkerStandalone
+          data={finalData}
+          markerLabel={kFormatter(finalData.reduce((p, c) => p + c.value, 0))}
+          {...sharedStandaloneMarkerProperties}
+        />
+      );
+    } else {
+      return (
+        <ChartMarkerStandalone
+          data={finalData}
+          markerLabel={mFormatter(finalData.reduce((p, c) => p + c.value, 0))}
+          dependentAxisLogScale={
+            activeMarkerConfiguration &&
+            'dependentAxisLogScale' in activeMarkerConfiguration
+              ? activeMarkerConfiguration.dependentAxisLogScale
+              : false
+          }
+          {...sharedStandaloneMarkerProperties}
+        />
+      );
+    }
+  }, [previewMarkerData]);


It may be possible to move the logic for generating continuousMarkerPreview inside the *MarkerConfigurationMenu components (as a utility function, I imagine). It looks like all but one (geoConfig) of the props needed to make previewMarkerData are already passed to the *MarkerConfigurationMenu components, so it wouldn't involve a lot of "prop bloat". In fact we'd be removing one prop (continuousMarkerPreview) and adding geoConfig, so breaking even!

It would clean things up a bit at this outer level.

I wonder if this should wait for #106?

packages/libs/eda/src/lib/map/analysis/appState.ts

…documentation

dmfalke

This is all looking really good. I reviewed the code and made a couple of suggestions. Feel free to comment if you disagree.

I will continue to think about the best way caching calls to the distribution service, but I think we should make it a follow-up task.

dmfalke · 2023-06-29T19:38:59Z

packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx

+  const { markersData: previewMarkerData } = useStandaloneMapMarkers({
+    boundsZoomLevel: undefined,
+    geoConfig: geoConfig,
+    studyId,
+    filters,
+    markerType,
+    selectedOverlayVariable: activeMarkerConfiguration?.selectedVariable,
+    overlayConfig: activeOverlayConfig.value,
+    outputEntityId: outputEntity?.id,
+  });
+
+  const continuousMarkerPreview = useMemo(() => {
+    if (!previewMarkerData || !previewMarkerData.length) return;
+    const initialDataObject = previewMarkerData[0].data.map((data) => ({
+      label: data.label,
+      value: 0,
+      ...(data.color ? { color: data.color } : {}),
+    }));
+    const typedData =
+      markerType === 'pie'
+        ? ([...previewMarkerData] as DonutMarkerProps[])
+        : ([...previewMarkerData] as ChartMarkerProps[]);
+    const finalData = typedData.reduce(
+      (prevData, currData) =>
+        currData.data.map((data, index) => ({
+          label: data.label,
+          value: data.value + prevData[index].value,
+          ...('color' in prevData[index]
+            ? { color: prevData[index].color }
+            : 'color' in data
+            ? { color: data.color }
+            : {}),
+        })),
+      initialDataObject
+    );
+    if (markerType === 'pie') {
+      return (
+        <DonutMarkerStandalone
+          data={finalData}
+          markerLabel={kFormatter(finalData.reduce((p, c) => p + c.value, 0))}
+          {...sharedStandaloneMarkerProperties}
+        />
+      );
+    } else {
+      return (
+        <ChartMarkerStandalone
+          data={finalData}
+          markerLabel={mFormatter(finalData.reduce((p, c) => p + c.value, 0))}
+          dependentAxisLogScale={
+            activeMarkerConfiguration &&
+            'dependentAxisLogScale' in activeMarkerConfiguration
+              ? activeMarkerConfiguration.dependentAxisLogScale
+              : false
+          }
+          {...sharedStandaloneMarkerProperties}
+        />
+      );
+    }
+  }, [previewMarkerData]);


I wonder if this should wait for #106?

dmfalke · 2023-06-29T19:45:01Z

packages/libs/eda/src/lib/map/analysis/MarkerConfiguration/BarPlotMarkerConfigurationMenu.tsx

+  useEffect(() => {
+    if (overlayConfiguration?.overlayType !== 'categorical') return;
+    setUncontrolledSelections(new Set(overlayConfiguration?.overlayValues));
+  }, [overlayConfiguration?.overlayValues, overlayConfiguration?.overlayType]);


You can remove the useEffect here. See https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes.

Suggested change

useEffect(() => {

if (overlayConfiguration?.overlayType !== 'categorical') return;

setUncontrolledSelections(new Set(overlayConfiguration?.overlayValues));

}, [overlayConfiguration?.overlayValues, overlayConfiguration?.overlayType]);

if (overlayConfiguration?.overlayType === 'categorical') {

setUncontrolledSelections(new Set(overlayConfiguration?.overlayValues));

}

This suggestion results in an infinite loop, which is why I think I reached for the useEffect. But it does appear that the "Better" suggestion from that section of the React docs works. Pretty clunky looking though!

const [previousOverlays, setPreviousOverlays] = useState(overlayConfiguration?.overlayValues); if ( previousOverlays !== overlayConfiguration?.overlayValues && overlayConfiguration?.overlayType === 'categorical' ) { setUncontrolledSelections(new Set(overlayConfiguration?.overlayValues)); setPreviousOverlays(overlayConfiguration.overlayValues); }

Thoughts on this?

Ah, yes, I see. This is more verbose, but also more efficient. Since this logic will be used in at least two places, maybe extract it into a hook. You can also add the initialization of uncontrolledSelections in the hook and call it useUncontrolledSelections. That will make it easier for the reader to ignore the implementation details, if they want to.

packages/libs/eda/src/lib/map/analysis/MarkerConfiguration/PieMarkerConfigurationMenu.tsx

dmfalke · 2023-06-29T19:53:05Z

packages/libs/eda/src/lib/map/analysis/utils/categoricalValues.ts

+    const unfilteredDistributionResponse =
+      await subsettingClient.getDistribution(
+        studyId,
+        overlayEntity.id,
+        overlayVariable.id,
+        {
+          valueSpec: 'count',
+          filters: [],
+        }
+      );


I don't have any suggestions right now, but I will think about it some more.

bobular · 2023-07-06T06:38:30Z

🎉

…

On Wed, 5 Jul 2023, 17:47 jeremy, ***@***.***> wrote: Merged #285 <#285> into main. — Reply to this email directly, view it on GitHub <#285 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACLLHY5PWBQJOSAVUDCNQLXOWLDHANCNFSM6AAAAAAY56EKWY> . You are receiving this because your review was requested.Message ID: ***@***.***>

jernestmyers added 10 commits June 6, 2023 14:03

Create component for configuring categorical markers

4d25b60

Wire up new table for barplot categorical marker configs

2416577

Add CategoricalMarkerConfigurationTable to donut config menu

37c7151

Clean up types related to allValues overlay data

e038f2e

Add a barplot marker preview for categorical vars

4d03d4a

Ensure 'All other values' is at end of selectedValues and clean up so…

1d3c312

…me code

Add donut marker preview for categorical marker config

7e49adb

Display histogram in marker config menu for continuous vars

a5188f7

Merge branch 'main' into 190-advanced-map-marker-config

ebd1ea0

Remove some ts-ignores

50bf469

jernestmyers mentioned this pull request Jun 9, 2023

Return markerProps from useStandaloneMapMarkers #292

Merged

Merge branch 'main' into 190-advanced-map-marker-config

64ce841

adnauseum reviewed Jun 9, 2023

View reviewed changes

jernestmyers added 7 commits June 12, 2023 14:13

Wire up marker preview for continuous vars

aad6f1b

Clean up some of the marker preview logic

5181895

Change from allValuesSorted to just allValues

784f218

Remove ts-ignores from advanced marker config work

3ed3ca2

Use standalone markers for all previews and add some styling

c5c37be

Tweak selectedValues logic and add documentation

fa7daad

Change type name for getValues function params

d4706b5

jernestmyers marked this pull request as ready for review June 13, 2023 15:48

jernestmyers requested review from bobular and dmfalke June 13, 2023 16:01

bobular reviewed Jun 14, 2023

View reviewed changes

bobular mentioned this pull request Jun 14, 2023

DonutMarker donut width needs to respond to markerScale #301

Closed

jernestmyers added 2 commits June 14, 2023 17:01

Wire up selectAll and deselectAll handlers

9e99c84

Add indeterminate state to (de)selectAll toggle

dd8049a

- apply deselectAll handler if indeterminate checkbox is clicked

jernestmyers added 2 commits June 26, 2023 15:44

Merge main and resolve conflicts

b991dae

Fix paths to coreui

0f08b40

bobular mentioned this pull request Jun 27, 2023

Map - issues with barplot marker previews #326

Closed

jernestmyers added 8 commits June 27, 2023 10:00

Improve naming and add number formatting

f634684

Remove allValues property and revert its related logic

6334e9b

Refactor categorical values logic and implement filtered/visible toggle

2b4e966

Remove ts-ignores

80faf09

Add log scale to categorical bar plot marker previews

0ba4e3e

Fix bug when deselecting values where 'All other values' label is irr…

3247e0a

…elevant

Implement column sorting and preserve sort state between filtered/vis…

923fbf5

…ible toggles

Refactor useStandaloneMapMarkers interface to remove isMarkerPreview …

121d986

…prop

jernestmyers commented Jun 28, 2023

View reviewed changes

Clean up and document code

0c6840e

bobular mentioned this pull request Jun 29, 2023

Map: advanced categorical marker config table could have "checked items at top" toggle #337

Open

bobular approved these changes Jun 29, 2023

View reviewed changes

Simplify logic and improve documentation

8338e5a

bobular assigned jernestmyers and unassigned jernestmyers Jun 29, 2023

jernestmyers added 4 commits June 29, 2023 11:51

Guard against TooManySelectionOverlay displaying erroneously and add …

cdb2aa3

…documentation

Remove magic number, improve logic and improve documentation

faff8cf

Fix bug in onMultipleRowSelect callback re: UNSELECTED_TOKEN value

69d5cf9

Clean up selectedValues type

cbdd30c

dmfalke requested changes Jun 29, 2023

View reviewed changes

Abstract uncontrolledSelections logic into a hook

87d8baa

jernestmyers requested a review from dmfalke June 30, 2023 16:34

dmfalke approved these changes Jul 5, 2023

View reviewed changes

Merge branch 'main' into 190-advanced-map-marker-config

b95bc83

jernestmyers merged commit 8e25821 into main Jul 5, 2023
1 check passed

jernestmyers deleted the 190-advanced-map-marker-config branch July 5, 2023 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement advanced map marker configuration menu #285

Implement advanced map marker configuration menu #285

jernestmyers commented Jun 7, 2023 •

edited

Loading

adnauseum Jun 9, 2023

jernestmyers Jun 9, 2023

adnauseum Jun 9, 2023 •

edited

Loading

bobular commented Jun 14, 2023

bobular left a comment •

edited

Loading

bobular commented Jun 14, 2023

jernestmyers commented Jun 14, 2023

jernestmyers Jun 28, 2023

bobular left a comment

bobular Jun 29, 2023

dmfalke Jun 29, 2023

bobular Jun 29, 2023

dmfalke Jun 29, 2023

dmfalke left a comment

dmfalke Jun 29, 2023

dmfalke Jun 29, 2023

jernestmyers Jun 29, 2023

dmfalke Jun 30, 2023

dmfalke Jun 29, 2023

bobular commented Jul 6, 2023 via email

		}

		// TODO: generalize this and PieMarkerConfigMenu into MarkerConfigurationMenu. Lots of code repitition...

Implement advanced map marker configuration menu #285

Implement advanced map marker configuration menu #285

Conversation

jernestmyers commented Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnauseum Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

bobular commented Jun 14, 2023

bobular left a comment • edited Loading

Choose a reason for hiding this comment

bobular commented Jun 14, 2023

jernestmyers commented Jun 14, 2023

Choose a reason for hiding this comment

bobular left a 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

dmfalke left a 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

bobular commented Jul 6, 2023 via email

jernestmyers commented Jun 7, 2023 •

edited

Loading

adnauseum Jun 9, 2023 •

edited

Loading

bobular left a comment •

edited

Loading