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

[ERA-7681] - Map Drawing Tools #725

Merged
merged 22 commits into from
Aug 16, 2022
Merged

[ERA-7681] - Map Drawing Tools #725

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
95b8762
half-assed implementation of useDrawToolGeoJson hook for a PoC.
JoshuaVulcan Aug 2, 2022
4c7a435
map layer and source hooks. map draw tool data hooks. abstracting rul…
JoshuaVulcan Aug 4, 2022
045b1de
removing unused 'autocomplete line' geojson calc
JoshuaVulcan Aug 5, 2022
8a71a46
test planning. code simplification.
JoshuaVulcan Aug 5, 2022
d2b315d
breaking up combined component code. test files.
JoshuaVulcan Aug 5, 2022
a7b0d7c
upgrading to React 18.2.0
JoshuaVulcan Aug 8, 2022
7705bfa
test scaffolding, switching to direct map context usage for draw tool…
JoshuaVulcan Aug 8, 2022
91be1e8
context refactor for tests
JoshuaVulcan Aug 9, 2022
81234d7
goodbye Layer and Source. hello hooks. lots of code cleanup in paymen…
JoshuaVulcan Aug 10, 2022
5cd996c
using renderHook to test (and scaffold tests for) new custom map inte…
JoshuaVulcan Aug 11, 2022
d444c15
fixing sidebar test scaffolding for more consistent results. cleaning…
JoshuaVulcan Aug 11, 2022
30b52ee
PR feedback, test tweaks
JoshuaVulcan Aug 11, 2022
964d485
test coverage across the board
JoshuaVulcan Aug 11, 2022
ee0a97f
guard clause for optional method
JoshuaVulcan Aug 11, 2022
008f6fa
guard clause for optional method
JoshuaVulcan Aug 11, 2022
98d5d8c
removing test focus
JoshuaVulcan Aug 11, 2022
bf4670e
Merge branch 'develop' into ERA-7681
JoshuaVulcan Aug 11, 2022
101b815
merging up develop, resolving conflicts, PR feedback tweaks
JoshuaVulcan Aug 12, 2022
437326b
bootstrap update to fix broken dropdowns
JoshuaVulcan Aug 12, 2022
ee3ccf1
undoing accidental lib updates in the wrong branch :-)
JoshuaVulcan Aug 15, 2022
d228b94
toggling drawing mode feature flag back to line
JoshuaVulcan Aug 16, 2022
f5e1ff1
merging up develop, resolving conflicts
JoshuaVulcan Aug 16, 2022
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
2 changes: 1 addition & 1 deletion .env.development
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
REACT_APP_DAS_HOST=https://develop.pamdas.org
REACT_APP_GA_TRACKING_ID=UA-12413928-16
REACT_APP_MOCK_EVENTS_API=true
REACT_APP_MOCK_EVENTS_API=false
13 changes: 7 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"humanize-duration": "^3.27.1",
"localforage": "^1.7.3",
"lodash-es": "^4.17.11",
"mapbox-gl": "^2.4.1",
"mapbox-gl": "^2.9.2",
"mb-isochrone": "^0.1.0",
"pluralize": "^8.0.0",
"prop-types": "^15.7.2",
Expand Down Expand Up @@ -72,6 +72,7 @@
"@testing-library/dom": "^8.1.0",
"@testing-library/jest-dom": "^5.14.1",
"@testing-library/react": "^12.0.0",
"@testing-library/react-hooks": "^8.0.1",
"@testing-library/user-event": "^13.0.16",
"eslint": "^8.10.0",
"eslint-config-airbnb": "^18.0.1",
Expand All @@ -92,10 +93,10 @@
"jest": {
"coverageThreshold": {
"global": {
"branches": 33.03,
"functions": 33.59,
"lines": 45.82,
"statements": 44.2
"branches": 37.11,
"functions": 37.74,
"lines": 49.51,
"statements": 48.01
}
},
"coverageReporters": [
Expand All @@ -110,7 +111,7 @@
],
"resolutions": {
"eslint-loader": "3.0.2",
"mapbox-gl": "2.4.1",
"mapbox-gl": "2.9.2",
"node-fetch": "2.6.7",
"immer": "9.0.6"
}
Expand Down
129 changes: 64 additions & 65 deletions src/AnalyzersLayer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import React, { memo } from 'react';
import { Source, Layer } from 'react-mapbox-gl';

import React, { memo, useMemo } from 'react';
import withMapViewConfig from '../WithMapViewConfig';

import { LAYER_IDS, SOURCE_IDS } from '../constants';
import { useMapEventBinding, useMapLayer, useMapSource } from '../hooks';

const { ANALYZER_POLYS_WARNING, ANALYZER_POLYS_CRITICAL, ANALYZER_LINES_WARNING,
ANALYZER_LINES_CRITICAL, SUBJECT_SYMBOLS } = LAYER_IDS;
Expand Down Expand Up @@ -63,68 +62,68 @@ const AnalyzerLayer = (
onAnalyzerGroupExit(e, hoverStateIds);
};

const warningLinesData = {
type: 'geojson',
data: warningLines,
};

const criticalLinesData = {
type: 'geojson',
data: criticalLines,
};

const warningPolysData = {
type: 'geojson',
data: warningPolys,
};

const criticalPolysData = {
type: 'geojson',
data: criticalPolys,
};

return <>
<Source id={ANALYZER_POLYS_WARNING_SOURCE} geoJsonSource={warningPolysData} />
<Source id={ANALYZER_POLYS_CRITICAL_SOURCE} geoJsonSource={criticalPolysData} />
<Source id={ANALYZER_LINES_CRITICAL_SOURCE} geoJsonSource={warningLinesData} />
<Source id={ANALYZER_LINES_WARNING_SOURCE} geoJsonSource={criticalLinesData} />

{/* due to a bug in mapboxgl, we need to treat polys as lines, to
get a dotted border line to appear*/}

<Layer minZoom={minZoom} sourceId={ANALYZER_POLYS_WARNING_SOURCE} type='line'
id={ANALYZER_POLYS_WARNING}
paint={linePaint}
onMouseEnter={onAnalyzerFeatureEnter}
onMouseLeave={onAnalyzerFeatureExit}
onClick={onAnalyzerFeatureClick} />

{isSubjectSymbolsLayerReady && <>
<Layer minZoom={minZoom} sourceId={ANALYZER_POLYS_CRITICAL_SOURCE} type='line'
before={SUBJECT_SYMBOLS}
id={ANALYZER_POLYS_CRITICAL}
paint={criticalLinePaint} layout={lineLayout}
onMouseEnter={onAnalyzerFeatureEnter}
onMouseLeave={onAnalyzerFeatureExit}
onClick={onAnalyzerFeatureClick} />

<Layer minZoom={minZoom} sourceId={ANALYZER_LINES_CRITICAL_SOURCE} type='line'
before={SUBJECT_SYMBOLS}
id={ANALYZER_LINES_WARNING}
paint={linePaint} layout={lineLayout}
onMouseEnter={onAnalyzerFeatureEnter}
onMouseLeave={onAnalyzerFeatureExit}
onClick={onAnalyzerFeatureClick} />

<Layer minZoom={minZoom} sourceId={ANALYZER_LINES_WARNING_SOURCE} type='line'
before={SUBJECT_SYMBOLS}
id={ANALYZER_LINES_CRITICAL}
paint={criticalLinePaint} layout={lineLayout}
onMouseEnter={onAnalyzerFeatureEnter}
onMouseLeave={onAnalyzerFeatureExit}
onClick={onAnalyzerFeatureClick} />
</>}
</>;
const layerConfig = useMemo(() => ({
before: SUBJECT_SYMBOLS,
minZoom,
condition: !!isSubjectSymbolsLayerReady,
}), [isSubjectSymbolsLayerReady, minZoom]);

useMapSource(ANALYZER_POLYS_WARNING_SOURCE, warningPolys);
useMapLayer(
ANALYZER_POLYS_WARNING,
'line',
ANALYZER_POLYS_WARNING_SOURCE,
linePaint,
undefined,
layerConfig,
);

useMapSource(ANALYZER_POLYS_CRITICAL_SOURCE, criticalPolys);
useMapLayer(
ANALYZER_POLYS_CRITICAL,
'line',
ANALYZER_POLYS_CRITICAL_SOURCE,
criticalLinePaint, lineLayout,
layerConfig,
);

useMapSource(ANALYZER_LINES_WARNING_SOURCE, warningLines);
useMapLayer(
ANALYZER_LINES_WARNING,
'line',
ANALYZER_LINES_WARNING_SOURCE,
linePaint,
lineLayout,
layerConfig,
);

useMapSource(ANALYZER_LINES_CRITICAL_SOURCE, criticalLines);
useMapLayer(
ANALYZER_LINES_CRITICAL,
'line',
ANALYZER_LINES_CRITICAL_SOURCE,
criticalLinePaint,
lineLayout,
layerConfig,
);

// (eventType = 'click', handlerFn = noop, layerId = null, condition = true)
useMapEventBinding('mouseenter', onAnalyzerFeatureEnter, ANALYZER_POLYS_WARNING);
useMapEventBinding('mouseenter', onAnalyzerFeatureEnter, ANALYZER_POLYS_CRITICAL);
useMapEventBinding('mouseenter', onAnalyzerFeatureEnter, ANALYZER_LINES_WARNING);
useMapEventBinding('mouseenter', onAnalyzerFeatureEnter, ANALYZER_LINES_CRITICAL);

useMapEventBinding('mouseleave', onAnalyzerFeatureExit, ANALYZER_POLYS_WARNING);
useMapEventBinding('mouseleave', onAnalyzerFeatureExit, ANALYZER_POLYS_CRITICAL);
useMapEventBinding('mouseleave', onAnalyzerFeatureExit, ANALYZER_LINES_WARNING);
useMapEventBinding('mouseleave', onAnalyzerFeatureExit, ANALYZER_LINES_CRITICAL);

useMapEventBinding('click', onAnalyzerFeatureClick, ANALYZER_POLYS_WARNING);
useMapEventBinding('click', onAnalyzerFeatureClick, ANALYZER_POLYS_CRITICAL);
useMapEventBinding('click', onAnalyzerFeatureClick, ANALYZER_LINES_WARNING);
useMapEventBinding('click', onAnalyzerFeatureClick, ANALYZER_LINES_CRITICAL);

return null;
};

export default memo(withMapViewConfig(AnalyzerLayer));
2 changes: 1 addition & 1 deletion src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ const App = (props) => {

<div className={`app-container ${sidebarOpen ? 'sidebar-open' : 'sidebar-closed'}`}>
<Map map={map} onMapLoad={onMapHasLoaded} socket={socket} pickingLocationOnMap={pickingLocationOnMap} />
{!!map && <SideBar map={map} />}
{!!map && <SideBar />}
<ModalRenderer map={map} />
</div>

Expand Down
58 changes: 39 additions & 19 deletions src/BaseLayerRenderer/TileLayerRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React, { memo, Fragment, useContext, useMemo, useEffect } from 'react';
import { MapContext, Layer, Source } from 'react-mapbox-gl';

import React, { memo, useContext, useMemo, useEffect } from 'react';
import { MapContext } from '../App';

import { TILE_LAYER_SOURCE_TYPES, LAYER_IDS, MAX_ZOOM, MIN_ZOOM } from '../constants';
import { useMapLayer, useMapSource } from '../hooks';

import { calcConfigForMapAndSourceFromLayer } from '../utils/layers';

Expand All @@ -14,13 +14,31 @@ const RASTER_SOURCE_OPTIONS = {
'tileSize': 256,
};

const RenderFunction = ({ children }) => <>{children}</>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm what's this for? Can't we just render the SourceComponent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope.
https://www.patterns.dev/posts/render-props-pattern/


const SourceComponent = ({ id, tileUrl, sourceConfig }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We ended up removing a component from that library, to add our own since we needed to call N hooks depending on an array. I know this involves a change to what you've done, but a possible solution that keep us away from patterns like this is to have the hooks working like a factory more than like a method:


  const createMapSource = useCreateMapSource();

  arrayWithUnknownLength.forEach((value) => createMapSource(value.id, null, config));

We only need to call the hook once and now we can create as many sources (and layers) as we want with a memoized function (so it doesn't trigger renders) :) Similar to const dispatch = useDispatch().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just render props, a well-known and proven React pattern. Iterative hook generation, however, is an anti-pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't know what I'm missing, but I don't see the render prop pattern here. As far as I know that pattern looks like this: <Component renderProp={() => <div>This function returning JSX is the render prop</div>} /> but here I just see a renderless component within a "render function" that is just rendering regular children within a fragment 🤔 (this paragraph is rendering the word render a few times jeje)

Iterative hook generation definitely sounds like an antipattern, my suggestion was not to call or generate hooks within a loop, but just call it once and get a memoized function that let us create layers and sources freely. I don't see why that is an antipattern, it's literally what standard hooks like useCallback or the setter of a useState do.

It was a suggestion though, I hadn't seen anything like this and it felt confusing at first, but if you like it that's ok.

Copy link
Collaborator Author

@JoshuaVulcan JoshuaVulcan Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah sort of -- except I'm using it for children à là the docs, where the child is a function. Then abstracting the child function further as a named component ("SourceComponent").

You're right that I misread your suggestion. I still like my pattern better 🙃 hahahaha but I'm open to discussing further if you have strong feelings

const config = useMemo(() => ({
...RASTER_SOURCE_OPTIONS,
tiles: [
tileUrl,
],
...sourceConfig,
}), [sourceConfig, tileUrl]);

useMapSource(id, null, config);

return null;
};

const TileLayerRenderer = (props) => {
const { layers, currentBaseLayer } = props;

const activeLayer = layers.find(({ id }) => id === currentBaseLayer.id);

const map = useContext(MapContext);

const activeLayer = useMemo(() =>
layers.find(({ id }) => id === currentBaseLayer.id)
, [currentBaseLayer.id, layers]);

const { mapConfig, sourceConfig } = useMemo(() =>
calcConfigForMapAndSourceFromLayer(currentBaseLayer)
, [currentBaseLayer]);
Expand All @@ -32,20 +50,22 @@ const TileLayerRenderer = (props) => {
}
}, [map, mapConfig]);

return <Fragment>
{layers
.filter(layer => TILE_LAYER_SOURCE_TYPES.includes(layer.attributes.type))
.map(layer => <Source key={layer.id} id={`layer-source-${layer.id}`} tileJsonSource={{
...RASTER_SOURCE_OPTIONS,
tiles: [
layer.attributes.url,
],
...sourceConfig,
}} >
</Source>)}

{!!activeLayer && <Layer before={FEATURE_FILLS} id={`tile-layer-${activeLayer.id}`} key={activeLayer.id} sourceId={`layer-source-${activeLayer.id}`} type="raster" />}
</Fragment>;
useMapLayer(
`tile-layer-${activeLayer?.id}`,
'raster',
`layer-source-${activeLayer?.id}`,
undefined,
undefined,
{ before: FEATURE_FILLS, condition: !!activeLayer }
);

return layers
.filter(layer => TILE_LAYER_SOURCE_TYPES.includes(layer.attributes.type))
.map(layer =>
<RenderFunction key={layer.id}>
<SourceComponent id={`layer-source-${layer.id}`} sourceConfig={sourceConfig} tileUrl={layer.attributes.url} />
</RenderFunction>
);
};

export default memo(TileLayerRenderer);
Loading