Add new analysis button to all analyses component in map

[chowington]

Closes #425

[dmfalke]

The button does not appear if I am in a new analysis for a study, and I don't have other saved analyses:

I suppose that's okay, but it threw me off when testing.

I do wonder if it would be better to put that button in the top-right, and to make it use the coreui primary theme color? That would address my comment above. For reference, this is what it looks like now:

[dmfalke]

This is what I was thinking. It would also avoid the changes to AllAnalysis, which I think might be nice. @bobular thoughts?

[bobular]

That looks like a great suggestion to me.

[chowington]

I've made the requested change. That is, I'm pretty sure--I haven't been able to test the final styling due to server issues ☹️

[dmfalke]

I think this is almost there.

The positioning of the icon in the button is off:

Also, see my comment about history.go(0) below.

ETA that image was taken in Chrome.

[dmfalke]

I think the issue here is related to a bug in appState. Here is a git diff that should address the issue. Once applied, you should be able to remove this line (history.go(0)).

diff --git a/packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx b/packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx
index ad70931ae4..ed27b7404e 100755
--- a/packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx
+++ b/packages/libs/eda/src/lib/map/analysis/MapAnalysis.tsx
@@ -212,8 +212,7 @@ interface Props {
 }
 
 export function MapAnalysis(props: Props) {
-  const analysisState = useAnalysis(props.analysisId, 'pass');
-  const appStateAndSetters = useAppState('@@mapApp@@', analysisState);
+  const appStateAndSetters = useAppState('@@mapApp@@', props.analysisId);
   const geoConfigs = useGeoConfig(useStudyEntities());
 
   if (geoConfigs == null || geoConfigs.length === 0)
@@ -230,7 +229,6 @@ export function MapAnalysis(props: Props) {
     <MapAnalysisImpl
       {...props}
       {...(appStateAndSetters as CompleteAppState)}
-      analysisState={analysisState}
       geoConfigs={geoConfigs}
     />
   );
diff --git a/packages/libs/eda/src/lib/map/analysis/appState.ts b/packages/libs/eda/src/lib/map/analysis/appState.ts
index af57d76ede..12ecb3baf4 100644
--- a/packages/libs/eda/src/lib/map/analysis/appState.ts
+++ b/packages/libs/eda/src/lib/map/analysis/appState.ts
@@ -5,10 +5,11 @@ import { isEqual } from 'lodash';
 import { useCallback, useEffect, useMemo, useRef } from 'react';
 import {
   AnalysisState,
+  useAnalysis,
   useGetDefaultVariableDescriptor,
-  useStudyMetadata,
 } from '../../core';
 import { VariableDescriptor } from '../../core/types/variable';
+import { isSavedAnalysis } from '../../core/utils/analysis';
 import { useGetDefaultTimeVariableDescriptor } from './hooks/eztimeslider';
 
 const LatLngLiteral = t.type({ lat: t.number, lng: t.number });
@@ -122,7 +123,17 @@ export const defaultViewport: AppState['viewport'] = {
   zoom: 1,
 };
 
-export function useAppState(uiStateKey: string, analysisState: AnalysisState) {
+export function useAppState(uiStateKey: string, analysisId?: string) {
+  const analysisState = useAnalysis(analysisId);
+
+  // make some backwards compatability updates to the appstate retrieved from the back end
+  const appStateCheckedRef = useRef(false);
+
+  useEffect(() => {
+    // flip bit when analysis id changes
+    appStateCheckedRef.current = false;
+  }, [analysisId]);
+
   const { analysis, setVariableUISettings } = analysisState;
   const appState = pipe(
     AppState.decode(
@@ -177,9 +188,6 @@ export function useAppState(uiStateKey: string, analysisState: AnalysisState) {
     [defaultVariable, defaultTimeVariable]
   );
 
-  // make some backwards compatability updates to the appstate retrieved from the back end
-  const appStateCheckedRef = useRef(false);
-
   useEffect(() => {
     if (appStateCheckedRef.current) return;
     if (analysis) {
@@ -243,6 +251,7 @@ export function useAppState(uiStateKey: string, analysisState: AnalysisState) {
 
   return {
     appState,
+    analysisState,
     setActiveMarkerConfigurationType: useSetter(
       'activeMarkerConfigurationType'
     ),

[chowington]

@dmfalke Is the above still valid given the investigations you made and posted about on Slack?

[dmfalke]

yes! 🙂

[chowington]

I made these changes, but the issue is still happening for me :(

[dmfalke]

I figured out why the issue is still happening. When we transition from a saved analysis to a new analysis, the ref appStateCheckedRef is set to false after the new unsaved analysis is returned by useAnalysis. I changed this to a useState variable, and that addresses the issue, since updating its value will trigger a rerender:

diff --git a/packages/libs/eda/src/lib/map/analysis/appState.ts b/packages/libs/eda/src/lib/map/analysis/appState.ts
index 2437ef7659..72fbc9b87f 100644
--- a/packages/libs/eda/src/lib/map/analysis/appState.ts
+++ b/packages/libs/eda/src/lib/map/analysis/appState.ts
@@ -2,7 +2,7 @@ import { getOrElseW } from 'fp-ts/lib/Either';
 import { pipe } from 'fp-ts/lib/function';
 import * as t from 'io-ts';
 import { isEqual } from 'lodash';
-import { useCallback, useEffect, useMemo, useRef } from 'react';
+import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
 import { useAnalysis, useGetDefaultVariableDescriptor } from '../../core';
 import { VariableDescriptor } from '../../core/types/variable';
 import { useGetDefaultTimeVariableDescriptor } from './hooks/eztimeslider';
@@ -122,11 +122,11 @@ export function useAppState(uiStateKey: string, analysisId?: string) {
   const analysisState = useAnalysis(analysisId);
 
   // make some backwards compatability updates to the appstate retrieved from the back end
-  const appStateCheckedRef = useRef(false);
+  const [appStateChecked, setAppStateChecked] = useState(false);
 
   useEffect(() => {
     // flip bit when analysis id changes
-    appStateCheckedRef.current = false;
+    setAppStateChecked(false);
   }, [analysisId]);
 
   const { analysis, setVariableUISettings } = analysisState;
@@ -184,7 +184,7 @@ export function useAppState(uiStateKey: string, analysisId?: string) {
   );
 
   useEffect(() => {
-    if (appStateCheckedRef.current) return;
+    if (appStateChecked) return;
     if (analysis) {
       if (!appState) {
         setVariableUISettings((prev) => ({
@@ -219,9 +219,9 @@ export function useAppState(uiStateKey: string, analysisId?: string) {
           }));
         }
       }
-      appStateCheckedRef.current = true;
+      setAppStateChecked(true);
     }
-  }, [analysis, appState, setVariableUISettings, uiStateKey, defaultAppState]);
+  }, [analysis, appState, setVariableUISettings, uiStateKey, defaultAppState, appStateChecked]);
 
   function useSetter<T extends keyof AppState>(key: T) {
     return useCallback(

[chowington]

That worked! Thanks Dave!

[chowington]

Ready for review

[chowington]

@dmfalke

[aurreco-uga]

thinking the header too, to leave the My analysis page the same as non-map eda.. (we could decide to change my analysis popup/page.. i would suggest to remove the button to delete unpinned and have always actions act on the selected analyses, have one icon/term per action so we can fit more actions.. )

[bobular]

thinking the header too

I'd agree if they were equivalent in size/weight to the pencil icon, and located just to the right of it.

Keep the + new analysis button in the My Analyses page though.

[dmfalke]

Changes look great, thanks!