Skip to content

Commit

Permalink
Fix delete button not working for static AOI + persist MapboxDraw mod…
Browse files Browse the repository at this point in the history
…e across reloads (#945)

**Related Ticket:**

Addressed [942](#942)

### Description of Changes
In our `aoisUpdateGeometryAtom` we assumed that each feature added on
the map via MapboxDraw is selected. This worked well until we started
using the `static_mode` for presets where we disable any interactions
attached to them. As a result, we introduced a state mismatch between
MapboxDraw (the preset features were marked as not selected) and what we
held in our atom in React (the preset features were marked as selected).

To fix this in the short-term, two changes were made:

- We switch to simple_select mode if static_mode has been selected when
using the trash icon
- A new url param has been added to track whether a present aoi should
be editable or is only there for analysis (static). This will track the
correct mode across page reloads

### Notes & Questions About Changes

Due to the urgency of the fix that should go out by EOW, I opted for the
fix in this PR because I wasn't sure if I'll manage to implement / test
a long-term solution without breaking something (the approach nicely
outlined
[here](#710 (comment)).
My understanding of a longer-term solution is as follows:

1. Introduce a `drawStateAtom` that keeps track of various draw modes,
features, selected features etc.
2. Add a custom hook `useSyncDraw` that reads/writes to the atom and
"syncs" the state between React and MapboxDraw
3. Add event handlers in the custom hook that update the atom as a
replacement of the current programmatic way

@hanbyul-here @sandrahoang686 If we ticket this for the upcoming
sprint/s, I can come up with a draft PR

### Validation / Testing
- Validate that hand-drawn and uploaded polygons can be edited and
deleted as usual
- Validate that presets can also be deleted
  • Loading branch information
dzole0311 authored May 13, 2024
2 parents 27b3670 + acffd90 commit a6d9dd7
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 22 deletions.
8 changes: 8 additions & 0 deletions app/scripts/components/common/map/controls/aoi/atoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,12 @@ export const aoiDeleteAllAtom = atom(null, (get, set) => {
set(aoisSerialized, encodeAois([]));
});

// Atom that tracks whether an AOI can be edited or not.
export const selectedForEditingAtom = atomWithUrlValueStability({
initialValue: (new URLSearchParams(window.location.search).get('selectedForEditing') !== 'false'),
urlParam: 'selectedForEditing',
hydrate: (value) => value !== 'false',
dehydrate: (value) => value ? 'true' : 'false'
});

export const isDrawingAtom = atom(false);
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, { useCallback, useEffect, useState } from 'react';
import { createPortal } from 'react-dom';
import { Feature, Polygon } from 'geojson';
import styled, { css } from 'styled-components';
import { useSetAtom } from 'jotai';
import { useAtom, useSetAtom } from 'jotai';
import bbox from '@turf/bbox';
import centroid from '@turf/centroid';
import {
Expand All @@ -19,14 +19,24 @@ import useMaps from '../../hooks/use-maps';
import useAois from '../hooks/use-aois';
import useThemedControl from '../hooks/use-themed-control';
import CustomAoIModal from './custom-aoi-modal';
import { aoiDeleteAllAtom } from './atoms';

import { aoiDeleteAllAtom, selectedForEditingAtom } from './atoms';
import PresetSelector from './preset-selector';
import { DIRECT_SELECT, DRAW_POLYGON, SIMPLE_SELECT, STATIC_MODE } from './';

import { TipToolbarIconButton } from '$components/common/tip-button';
import { Tip } from '$components/common/tip';
import { getZoomFromBbox } from '$components/common/map/utils';
import { ShortcutCode } from '$styles/shortcut-code';

// 'moving' feature is disabled, match the cursor style accoringly
export const aoiCustomCursorStyle = css`
&.mode-${STATIC_MODE} .mapboxgl-canvas-container,
&.feature-feature.mouse-drag .mapboxgl-canvas-container,
&.mouse-move .mapboxgl-canvas-container {
cursor: default;
}
`;

const AnalysisToolbar = styled(Toolbar)<{ visuallyDisabled: boolean }>`
background-color: ${themeVal('color.surface')};
border-radius: ${themeVal('shape.rounded')};
Expand Down Expand Up @@ -67,19 +77,25 @@ function CustomAoI({
const [presetIds, setPresetIds] = useState([]);
const [fileUploadedIds, setFileUplaodedIds] = useState([]);

const [selectedForEditing, setSelectedForEditing] = useAtom(selectedForEditingAtom);

const { onUpdate, isDrawing, setIsDrawing, features } = useAois();
const aoiDeleteAll = useSetAtom(aoiDeleteAllAtom);

// Needed so that this component re-renders to when the draw selection changes
// from feature to point.
const [, forceUpdate] = useState(0);
useEffect(() => {
const mbDraw = map?._drawControl;
if (!mbDraw) return;
const aoiSelectedFor = selectedForEditing ? SIMPLE_SELECT : STATIC_MODE;
mbDraw.changeMode(aoiSelectedFor);
const onSelChange = () => forceUpdate(Date.now());
map.on('draw.selectionchange', onSelChange);
return () => {
map.off('draw.selectionchange', onSelChange);
};
}, []);
}, [map, selectedForEditing]);

const resetAoisOnMap = useCallback(() => {
const mbDraw = map?._drawControl;
Expand Down Expand Up @@ -111,14 +127,14 @@ function CustomAoI({
if (!mbDraw) return;

if (fileUploadedIds.length) {
mbDraw.changeMode('simple_select', {
mbDraw.changeMode(SIMPLE_SELECT, {
featureIds: fileUploadedIds
});
mbDraw.trash();
}

if (presetIds.length) {
mbDraw.changeMode('simple_select', {
mbDraw.changeMode(SIMPLE_SELECT, {
featureIds: presetIds
});
mbDraw.trash();
Expand All @@ -145,11 +161,12 @@ function CustomAoI({
zoom: getZoomFromBbox(bounds)
});
const addedAoisId = mbDraw.add(fc);
mbDraw.changeMode('simple_select', {
mbDraw.changeMode(STATIC_MODE, {
featureIds: addedAoisId
});
setFileUplaodedIds(addedAoisId);
},[map, onUpdate, resetForFileUploaded]);
setSelectedForEditing(false);
},[map, onUpdate, resetForFileUploaded, setSelectedForEditing]);

const onPresetConfirm = useCallback((features: Feature<Polygon>[]) => {
const mbDraw = map?._drawControl;
Expand All @@ -168,17 +185,18 @@ function CustomAoI({
});
const pids = mbDraw.add(fc);
setPresetIds(pids);
mbDraw.changeMode('simple_static', {
mbDraw.changeMode(STATIC_MODE, {
featureIds: pids
});

},[map, onUpdate, resetForPresetSelect]);
setSelectedForEditing(false);
},[map, onUpdate, resetForPresetSelect, setSelectedForEditing]);

const toggleDrawing = useCallback(() => {
const mbDraw = map?._drawControl;
if (!mbDraw) return;
resetForDrawingAoi();
setIsDrawing(!isDrawing);
setSelectedForEditing(true);
}, [map, isDrawing, setIsDrawing, resetForDrawingAoi]);

const onTrashClick = useCallback(() => {
Expand All @@ -196,15 +214,22 @@ function CustomAoI({
// trigger the delete for the whole feature.
const selectedFeatures = mbDraw.getSelected()?.features;
if (
mbDraw.getMode() === 'direct_select' &&
mbDraw.getMode() === DIRECT_SELECT &&
selectedFeatures.length &&
!mbDraw.getSelectedPoints().features.length
) {
// Change mode so that the trash action works.
mbDraw.changeMode('simple_select', {
mbDraw.changeMode(SIMPLE_SELECT, {
featureIds: selectedFeatures.map((f) => f.id)
});
}
// If we are in static mode, we need to change to simple_select to be able
// to delete those features
if (mbDraw.getMode() === STATIC_MODE) {
mbDraw.changeMode(SIMPLE_SELECT, {
featureIds: features.map((f) => f.id)
});
}
// If nothing selected, delete all.
if (features.every((f) => !f.selected)) {
mbDraw.deleteAll();
Expand Down Expand Up @@ -309,9 +334,9 @@ export default function CustomAoIControl({
if (!mbDraw) return;

if (isDrawing) {
mbDraw.changeMode('draw_polygon');
mbDraw.changeMode(DRAW_POLYGON);
} else {
mbDraw.changeMode('simple_select', {
mbDraw.changeMode(SIMPLE_SELECT, {
featureIds: mbDraw.getSelectedIds()
});
}
Expand Down
11 changes: 8 additions & 3 deletions app/scripts/components/common/map/controls/aoi/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import { computeDrawStyles } from './style';

type DrawControlProps = MapboxDraw.DrawOptions;

export const STATIC_MODE = 'static_mode';
export const SIMPLE_SELECT = 'simple_select';
export const DIRECT_SELECT = 'direct_select';
export const DRAW_POLYGON = 'draw_polygon';

// Overriding the dragMove and dragFeature methods of the
// 'simple_select' and the 'direct_select' modes to avoid
// accidentally dragging the selected or hand-drawn AOIs
Expand All @@ -36,9 +41,9 @@ export default function DrawControl(props: DrawControlProps) {
styles: computeDrawStyles(theme),
modes: {
...MapboxDraw.modes,
simple_static: StaticMode,
simple_select: customSimpleSelect,
direct_select: customDirectSelect
[STATIC_MODE]: StaticMode,
[SIMPLE_SELECT]: customSimpleSelect,
[DIRECT_SELECT]: customDirectSelect
},
...props
});
Expand Down
5 changes: 3 additions & 2 deletions app/scripts/components/common/map/controls/hooks/use-aois.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
aoisUpdateGeometryAtom,
isDrawingAtom
} from '../aoi/atoms';
import { SIMPLE_SELECT } from '../aoi';

export default function useAois() {
const features = useAtomValue(aoisFeaturesAtom);
Expand Down Expand Up @@ -52,7 +53,7 @@ export default function useAois() {
);

const onDrawModeChange = useCallback((e) => {
if (e.mode === 'simple_select') {
if (e.mode === SIMPLE_SELECT) {
setIsDrawing(false);
}
}, [setIsDrawing]);
Expand All @@ -67,4 +68,4 @@ export default function useAois() {
isDrawing,
setIsDrawing
};
}
}
2 changes: 2 additions & 0 deletions app/scripts/components/common/map/maps.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { ExtendedStyle, Styles } from './styles';
import useMapCompare from './hooks/use-map-compare';
import MapComponent from './map-component';
import useMaps, { useMapsContext } from './hooks/use-maps';
import { aoiCustomCursorStyle } from './controls/aoi/custom-aoi-control';
import { COMPARE_CONTAINER_NAME, CONTROLS_CONTAINER_NAME } from '.';

const chevronRightURI = () =>
Expand Down Expand Up @@ -51,6 +52,7 @@ const MapsContainer = styled.div`
&.mouse-move .mapboxgl-canvas-container {
cursor: move;
}
${aoiCustomCursorStyle}
}
.mapboxgl-compare .compare-swiper-vertical {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,6 @@ export function AnalysisMessage({ mainMap }: { mainMap: MapRef | undefined }) {
setObsolete();
}, [setObsolete, features]);



const analysisCallback = useCallback(() => {
// Fit AOI
const bboxToFit = bbox({
Expand Down

0 comments on commit a6d9dd7

Please sign in to comment.