[ERA-7681] - Map Drawing Tools #725

JoshuaVulcan · 2022-08-04T22:52:09Z

https://allenai.atlassian.net/browse/ERA-7681

Refactoring the map ruler code for use as drawing tools.

…er componentry into drawing components. polygon and line segment draw modes.

… render layers

luixlive

I didn't review the ruler files, just the whole abstraction. I added several suggestions and comments, most of them trivial stuff. I like the whole approach!

luixlive · 2022-08-08T21:20:24Z

src/hooks/index.js

@@ -49,3 +51,105 @@ export const useDeepCompareEffect = (callback, dependencies) => {
    }
  }, [callback, dependencies]);
 };
+
+
+export const useMapEventBinding = (eventType = 'click', handlerFn = noop, layerId = null, condition = true) => {


Do you think it's a good idea to add click as the default event type and a noops as the default handler? It feels like we should always define explicitly these arguments when calling this hook so it throws an error if we accidentally miss something.

I honestly don't think it matters much -- errors will just fail, meaning test coverage would fail as well in expecting side effects. I like the inline self-documentation side of this, but if you feel strongly I can be persuaded to remove.

luixlive · 2022-08-08T21:22:34Z

src/hooks/index.js

+      return () => {
+        map.off(...args);
+      };
+    } else {


If map is undefined we will fall in this else, triggering map.off(...args); which would throw an exception. I think this else is not really necessary, just having the on / off methods trigger inside the if should keep the memory clean.

Also, in that case, we could take args inside the if so we don't run the .filter operation when it's not necessary.

luixlive · 2022-08-08T21:23:20Z

src/hooks/index.js

+
+export const useMapSource = (sourceId, data, config = { type: 'geojson' }) => {
+  const map = useContext(MapContext);
+  let source = map.getSource(sourceId);


I think this could be a const

luixlive · 2022-08-08T21:23:41Z

src/hooks/index.js

+  let source = map.getSource(sourceId);
+
+  useEffect(() => {
+    if (map) {


Two ifs that could become one :) map && !source

luixlive · 2022-08-08T21:24:41Z

src/hooks/index.js

+  useEffect(() => {
+    return () => {
+      setTimeout(() => {
+        map.getSource(sourceId) && map.removeSource(sourceId);


We already have map.getSource(sourceId) in the variable source, no need to fetch it again 👍

luixlive · 2022-08-08T21:56:13Z

src/MapDrawingTools/index.js

+  const [pointerLocation, setPointerLocation] = useState(null);
+
+  const cursorPopupCoords = useMemo(() => pointerLocation ? [pointerLocation.lng, pointerLocation.lat] : points[points.length - 1], [pointerLocation, points]);
+  const data = useDrawToolGeoJson(points, (drawing && cursorPopupCoords), drawingMode);


Again, I don't think data is a good name for this. We could follow the hook convention drawToolGeoJson, but honestly I'd rather receive the two properties spread: { drawnLineSegmentsGeoJson, fillPolygonGeoJson } so it's really clear what we are handling here.

I like it for the dataContainer, in passing up the data on eventing. for the MapLayers component I agree, it can be broken apart as props and passed explicitly. Will reactor.

luixlive · 2022-08-08T21:57:32Z

src/MapDrawingTools/index.js

+
+    e.preventDefault();
+    e.originalEvent.stopPropagation();
+    onMapClick.cancel();


What does this cancel do?

This ensures we don't have funny sync-related issues where a second click fires. It's ported over from the existing ruler code

luixlive · 2022-08-08T21:59:00Z

src/MapDrawingTools/index.js

+  return <>
+    {drawing && <CursorPopup coords={cursorPopupCoords} lineLength={lineLength} points={points} />}
+    <MapLayers data={data} />
+    {children}


I don't see the purpose of supporting children here if we are not going to wrap them.

For ❇️ the future ❇️ but it's trivial to add/remove

luixlive · 2022-08-08T22:01:44Z

src/MapDrawingTools/utils.js

+  return lineSegments;
+};
+
+export const createFillPolygonForCoords = (coords) => polygon([coords]);


I guess we have this trivial method just to keep it in utils with createLineSegmentGeoJsonForCoords even though it seems unnecessary. I'd just mention that it would be great to follow the same name convention, we are missing GeoJson here

luixlive · 2022-08-08T22:03:12Z

src/MapDrawingTools/index.js

+  points: PropTypes.array,
+};
+
+const CursorPopup = (props) => {


I think it would be cleaner to have this one in its own file, with its own tests and all. What do you think?

…t of technical debt.

…gration hooks.

… up empty files, typo correction, build improvements

luixlive

Another pass, a few comments.

luixlive · 2022-08-11T19:17:20Z

src/BaseLayerRenderer/TileLayerRenderer.js

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

+const RenderFunction = ({ children }) => <>{children}</>;


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

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

luixlive · 2022-08-11T19:22:52Z

src/BaseLayerRenderer/TileLayerRenderer.js

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

+const RenderFunction = ({ children }) => <>{children}</>;
+
+const SourceComponent = ({ id, tileUrl, sourceConfig }) => {


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().

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

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.

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

luixlive · 2022-08-11T19:27:42Z

src/hooks/index.js

@@ -1,6 +1,12 @@
-import { useState, useEffect, useRef } from 'react';


I'm not a big fan of having this hooks file which could grow infinitely larger with more and more hooks. This also involves having a single test file with lots of tests covering different stuff. We could have each hook file and its test in a separate folder and import / export them in the index, so we don't have to go deep in the folder structure when we want to import them. What do you think?

Eventually, yes. I don't think we're there yet, though it would be nice. If it's easy, which it should be, I'll break it up a bit more

luixlive · 2022-08-11T19:30:46Z

src/FeatureLayer/index.js

-    type: 'geojson',
-    data: polygons,
-  };
+  useMapSource('feature-line-source', lines);


I guess this technical debt was already there, but it should be nice to have these source names as constants.

Agreed! That's pretty trivial, I'll hit that on the final pass.

src/HeatLayer/index.js

src/MapDrawingTools/MapLayers.js

JoshuaVulcan · 2022-08-11T22:43:56Z

src/MapRulerControl/index.js

+      {drawing && <PointPopup map={map} points={points} pointIndex={points.length - 1} drawing={drawing} onClickFinish={onFinish} />}
+      {!drawing && popupPointSelected && <PointPopup map={map} points={points} pointIndex={selectedPointIndex} drawing={drawing} />}
+    </>}
+    {active && <MapDrawingTools drawing={drawing} drawingMode={DRAWING_MODES.POLYGON} points={points} onChange={onDrawChange} onClickPoint={onClickPoint} />}


@Alcoto95 @luixlive

We can consider drawingMode={DRAWING_MODES.POLYGON} the feature flag for testing the polygon side of things -- feel free to toggle it back to DRAWING_MODES.LINE to see the regular ol' ruler

luixlive

Great! I think there are still a few things in the previous comments pending to be implemented, but given our agreements and the code, I'm ready to approve this 💯

luixlive · 2022-08-12T15:05:53Z

src/BaseLayerRenderer/TileLayerRenderer.js

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

+const RenderFunction = ({ children }) => <>{children}</>;
+
+const SourceComponent = ({ id, tileUrl, sourceConfig }) => {


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.

Alcoto95 · 2022-08-12T16:09:26Z

Sometimes it doesn't allow you to add the second point 🤔 is this something that should work in this change?

You will see in this video that I am trying to add the second point a bit above the first one, and as it doesn't allow me to do so, I add it below, which it seems to be allowed.

And it happens all the time, if you add a point, then another one below that first point, and try to add another one above the two of them, it just doesn't allow to do so.

And you can see that the pointer changes from the finger pointer to the cross pointer moving the cursos as anyone would (not being fast on purpose). So I think that is causing us not to be able to add another point.

Screen.Recording.2022-08-12.at.11.04.35.mov

JoshuaVulcan · 2022-08-12T16:36:44Z

Sometimes it doesn't allow you to add the second point 🤔 is this something that should work in this change?

You will see in this video that I am trying to add the second point a bit above the first one, and as it doesn't allow me to do so, I add it below, which it seems to be allowed.

And it happens all the time, if you add a point, then another one below that first point, and try to add another one above the two of them, it just doesn't allow to do so.

And you can see that the pointer changes from the finger pointer to the cross pointer moving the cursos as anyone would (not being fast on purpose). So I think that is causing us not to be able to add another point.

Screen.Recording.2022-08-12.at.11.04.35.mov

This is caused by this bug thanks for our 3D-enabled maps 🙁
We can disable 3d map terrain ("settings" --> uncheck "3d map terrain") for now to ensure the drawing works correctly.
I'll keep cracking on a solution in the meantime; would you consider it a blocker? It's a bug in a library, not our code, and doesn't block functionality with a workaround. Not great though either...

Alcoto95

Let's move on with this one, we know there's an open issue for mapbox that affects this implementation, and there's a simple workaround that helps, just turning 3D Map Terrain Off.

JoshuaVulcan added 2 commits August 2, 2022 15:59

half-assed implementation of useDrawToolGeoJson hook for a PoC.

95b8762

map layer and source hooks. map draw tool data hooks. abstracting rul…

4c7a435

…er componentry into drawing components. polygon and line segment draw modes.

JoshuaVulcan requested review from luixlive and Alcoto95 August 4, 2022 22:52

JoshuaVulcan marked this pull request as draft August 4, 2022 22:52

JoshuaVulcan added 5 commits August 4, 2022 21:48

removing unused 'autocomplete line' geojson calc

045b1de

test planning. code simplification.

8a71a46

breaking up combined component code. test files.

d2b315d

upgrading to React 18.2.0

a7b0d7c

test scaffolding, switching to direct map context usage for draw tool…

7705bfa

… render layers

luixlive reviewed Aug 8, 2022

View reviewed changes

JoshuaVulcan added 4 commits August 9, 2022 12:37

context refactor for tests

91be1e8

goodbye Layer and Source. hello hooks. lots of code cleanup in paymen…

81234d7

…t of technical debt.

using renderHook to test (and scaffold tests for) new custom map inte…

5cd996c

…gration hooks.

fixing sidebar test scaffolding for more consistent results. cleaning…

d444c15

… up empty files, typo correction, build improvements

luixlive reviewed Aug 11, 2022

View reviewed changes

JoshuaVulcan added 5 commits August 11, 2022 13:03

PR feedback, test tweaks

30b52ee

test coverage across the board

964d485

guard clause for optional method

ee0a97f

guard clause for optional method

008f6fa

removing test focus

98d5d8c

JoshuaVulcan marked this pull request as ready for review August 11, 2022 22:42

JoshuaVulcan requested a review from luixlive August 11, 2022 22:42

JoshuaVulcan changed the title ~~[ERA-7681]~~ [ERA-7681] - Map Drawing Tools Aug 11, 2022

JoshuaVulcan commented Aug 11, 2022

View reviewed changes

Merge branch 'develop' into ERA-7681

bf4670e

luixlive approved these changes Aug 12, 2022

View reviewed changes

merging up develop, resolving conflicts, PR feedback tweaks

101b815

Alcoto95 approved these changes Aug 12, 2022

View reviewed changes

JoshuaVulcan added 4 commits August 12, 2022 15:14

bootstrap update to fix broken dropdowns

437326b

undoing accidental lib updates in the wrong branch :-)

ee3ccf1

toggling drawing mode feature flag back to line

d228b94

merging up develop, resolving conflicts

f5e1ff1

JoshuaVulcan merged commit 7b9d9a2 into develop Aug 16, 2022

JoshuaVulcan deleted the ERA-7681 branch August 16, 2022 04:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ERA-7681] - Map Drawing Tools #725

[ERA-7681] - Map Drawing Tools #725

JoshuaVulcan commented Aug 4, 2022

luixlive left a comment

luixlive Aug 8, 2022

JoshuaVulcan Aug 11, 2022

luixlive Aug 8, 2022

luixlive Aug 8, 2022

luixlive Aug 8, 2022

luixlive Aug 8, 2022

luixlive Aug 8, 2022

luixlive Aug 8, 2022

JoshuaVulcan Aug 11, 2022

luixlive Aug 8, 2022

JoshuaVulcan Aug 9, 2022

luixlive Aug 8, 2022

JoshuaVulcan Aug 9, 2022

luixlive Aug 8, 2022

luixlive Aug 8, 2022

JoshuaVulcan Aug 9, 2022

luixlive left a comment

luixlive Aug 11, 2022

JoshuaVulcan Aug 11, 2022

luixlive Aug 11, 2022

JoshuaVulcan Aug 11, 2022

luixlive Aug 12, 2022

JoshuaVulcan Aug 12, 2022 •

edited

Loading

luixlive Aug 11, 2022

JoshuaVulcan Aug 11, 2022

luixlive Aug 11, 2022

JoshuaVulcan Aug 11, 2022

JoshuaVulcan Aug 11, 2022

luixlive left a comment

luixlive Aug 12, 2022

Alcoto95 commented Aug 12, 2022

JoshuaVulcan commented Aug 12, 2022

Alcoto95 left a comment

		@@ -1,6 +1,12 @@
		import { useState, useEffect, useRef } from 'react';

[ERA-7681] - Map Drawing Tools #725

[ERA-7681] - Map Drawing Tools #725

Conversation

JoshuaVulcan commented Aug 4, 2022

luixlive 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

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

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

luixlive 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

JoshuaVulcan Aug 12, 2022 • edited Loading

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

luixlive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alcoto95 commented Aug 12, 2022

JoshuaVulcan commented Aug 12, 2022

Alcoto95 left a comment

Choose a reason for hiding this comment

JoshuaVulcan Aug 12, 2022 •

edited

Loading