Add FeatureSelection tool #271

padawannn · 2022-01-11T14:08:41Z

Name refactor (https://app.shortcut.com/cartoteam/story/204565/rename-drawingtoolwidget-and-drawingtoollayer). [sc-204565]
Google Maps compatibility.
Mask extension integration

coveralls · 2022-01-11T14:12:41Z

Pull Request Test Coverage Report for Build 1778848646

16 of 18 (88.89%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 70.388%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/react-redux/src/slices/cartoSlice.js	4	6	66.67%

Totals
Change from base Build 1757917925:	0.07%
Covered Lines:	1164
Relevant Lines:	1557

💛 - Coveralls

VictorVelarde · 2022-01-11T17:24:13Z

packages/react-redux/src/types.d.ts

-  drawingToolMode: typeof DRAW_MODES
+  drawingToolMode: string


Why such change here? isn't it better to use a more restricted type?

Because typeof DRAW_MODES doesn't work when I integrate it in typescript project. I accept suggestions

VictorVelarde · 2022-01-11T17:25:32Z

packages/react-redux/src/slices/cartoSlice.d.ts

@@ -149,3 +149,5 @@ export function setDrawingToolEnabled(enabled: boolean): {
 };

 export function selectSpatialFilter(state: any, sourceId?: string): object | null;
+
+export function selectDrawingToolMode(state: any): string | false;


If 'false' is equivalent to 'None', I think it could be better to add that option to an enum or type. What do you think?

selectDrawingToolMode is a selector that return false if the spatialFilter is null in the store (the default state). I think that creating another enum type could be over-engineering

I just see there are other options without adding boolean to the mix:

a) just an enum / type: DrawingToolMode.Type1, DrawingToolMode.Type2,... + DrawingToolMode.None, or more simple

b) just respecting the null for 'None'.

"selectDrawingToolMode" doesn't sound like a boolean to me, but a value from a predefined list. If required, we can later do in code something like this

const noFilter = selectDrawingToolMode === null if (noFilter) ...

What do you think?

I've done the b option

VictorVelarde · 2022-01-11T17:27:42Z

packages/react-widgets/src/layers/EditableCartoGeoJsonLayer.js

@@ -0,0 +1,30 @@
+import { EditableGeoJsonLayer } from '@nebula.gl/layers';
+
+const EVENT_TYPES = ['anyclick', 'pointermove', 'panstart', 'panmove', 'panend', 'keyup'];


Probably a comment with a bit of explanation on this would be really useful, about how this should be better a mechanism in nebula itself, with probably a PR on that in the future

Agree, I'm going to add a link to the original code and write a small explanation

VictorVelarde · 2022-01-11T17:28:55Z

packages/react-widgets/src/layers/EditableCartoGeoJsonLayer.js

+    for (const eventType of EVENT_TYPES) {
+      eventManager.on(eventType, eventHandler, {
+        // give nebula a higher priority so that it can stop propagation to deck.gl's map panning handlers
+        priority: 100


Is 100 the "top priority" or how does is work? I mean, is it an arbitrary number?.

I really don't know XD. This code has been copied from the original nebula code
https://github.com/uber/nebula.gl/blob/master/modules/layers/src/layers/editable-layer.ts#L83

XD, ok then add this link to code...

VictorVelarde

:+1

VictorVelarde · 2022-01-18T18:03:11Z

@padawannn I'm not sure if you prefer to merge this one together with #278, but I'm ok with this one, as it is now.

Is it mergeable independently? (if that's the case add changelog entry before)

padawannn · 2022-01-19T08:21:49Z

@padawannn I'm not sure if you prefer to merge this one together with #278, but I'm ok with this one, as it is now.

Is it mergeable independently? (if that's the case add changelog entry before)

I prefer to merge #278 first due to the name refactor requested by Borja

VictorVelarde · 2022-01-19T11:46:04Z

Ok, then adding this as 'blocked' for the moment

Josmorsot

LGTM, just minor question/suggestion

Josmorsot · 2022-01-28T11:00:16Z

packages/react-widgets/src/layers/useEventManager.js

+  isSelected,
+  selectedMode
+}) {
+  const isGmaps = useSelector((state) => BASEMAPS[state.carto.basemap].type === 'gmaps');


Is not 'gmaps' in any constant? Should we move it to a new one?

We don't have a constant for 'gmaps'. Move it to a new one it's not as easy as it sounds because the basemap package also uses that string so probably we would need to add it to the core package and do a refactor. @VictorVelarde maybe we could do it in another PR

bbecquet · 2022-01-28T12:10:08Z

packages/react-basemaps/src/basemaps/GoogleMap.js

  };

+  // Set layers should be outside of the useEffect to avoid problems with layers that changes


Won't it cause useless redraws on Deck?

Also, is it normal that the useEffect below has no dependency array?

Won't it cause useless redraws on Deck?

No, in fact, Deck always should receive a new instance of all layers and it decides if it's necessary to redraw or not. The creation of the layers or setting them within a useEffect is a common mistake. Check this example https://deck.gl/docs/get-started/using-with-react#the-deckgl-react-component

Also, is it normal that the useEffect below has no dependency array?
In this case yes although I admit that it is difficult to understand. In any case, this PR does not intend to modify that part of the code

packages/react-widgets/src/layers/FeatureSelectionLayer.js

bbecquet

LGTM

CHANGELOG.md

shortcut-integration · 2022-01-28T16:01:13Z

This pull request has been linked to Shortcut Story #204565: Rename DrawingToolWidget and DrawingToolLayer.

…toDB/carto-react into drawingTool-googleMaps-compatibility

Co-authored-by: Víctor Velarde <victor.velarde@gmail.com>

VictorVelarde

:rocket Well done

extend EditableGeoJsonLayer to allow send an eventManager as a prop

aa1e751

padawannn requested a review from VictorVelarde January 11, 2022 14:08

VictorVelarde reviewed Jan 11, 2022

View reviewed changes

padawannn added 2 commits January 12, 2022 16:48

minor changes

2ac4f1d

bug fix

e340c32

VictorVelarde requested review from bbecquet and eamador January 18, 2022 18:01

VictorVelarde approved these changes Jan 18, 2022

View reviewed changes

VictorVelarde added the builder label Jan 18, 2022

VictorVelarde added the blocked label Jan 19, 2022

padawannn added 7 commits January 24, 2022 10:34

finish google maps compatibility and added the mask extension (#278)

5f55f19

Merge branch 'master' into drawingTool-googleMaps-compatibility

04f1f9f

refactor

224154a

wip

6ee1a40

wip

e73fd6f

test

74b97fd

wip

cbafc72

padawannn requested a review from VictorVelarde January 25, 2022 13:07

padawannn added 5 commits January 25, 2022 14:29

add google map effect

0b13376

wip

3052490

new refactor

1cef257

conflicts

d745cc0

changelog

f0bbb9a

padawannn changed the title ~~Compatibility between the drawingTool and Google Maps (first part)~~ FeatureSelection Jan 27, 2022

padawannn changed the title ~~FeatureSelection~~ Feature selection Jan 27, 2022

padawannn marked this pull request as draft January 27, 2022 10:53

padawannn added 2 commits January 27, 2022 14:48

mask layer

aafbac8

remove unused imports

cd4c0ef

Josmorsot approved these changes Jan 28, 2022

View reviewed changes

bbecquet reviewed Jan 28, 2022

View reviewed changes

Update FeatureSelectionLayer.js

2f5354e

bbecquet approved these changes Jan 28, 2022

View reviewed changes

VictorVelarde reviewed Jan 28, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

VictorVelarde self-requested a review January 28, 2022 16:01

padawannn and others added 7 commits February 1, 2022 10:54

update deck gl version

fab0cf7

change log

30e7c45

Merge branch 'drawingTool-googleMaps-compatibility' of github.com:Car…

70056a0

…toDB/carto-react into drawingTool-googleMaps-compatibility

Update CHANGELOG.md

1c7484f

Co-authored-by: Víctor Velarde <victor.velarde@gmail.com>

change log

2f414d1

change log

396562c

update deck version in all packages

a5cfaab

padawannn marked this pull request as ready for review February 1, 2022 10:10

padawannn removed the blocked label Feb 1, 2022

Modify changelog entries

e7be25b

VictorVelarde self-assigned this Feb 1, 2022

VictorVelarde changed the title ~~Feature selection~~ Add FeatureSelection tool Feb 1, 2022

Add changelog entry

af39813

VictorVelarde approved these changes Feb 1, 2022

View reviewed changes

Bump internal dependencies among packages

52b48a7

VictorVelarde force-pushed the drawingTool-googleMaps-compatibility branch from 97d1669 to 52b48a7 Compare February 1, 2022 14:34

VictorVelarde merged commit 5f78921 into master Feb 1, 2022

VictorVelarde deleted the drawingTool-googleMaps-compatibility branch February 1, 2022 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FeatureSelection tool #271

Add FeatureSelection tool #271

padawannn commented Jan 11, 2022 •

edited by VictorVelarde

Loading

coveralls commented Jan 11, 2022 •

edited

Loading

VictorVelarde Jan 11, 2022

padawannn Jan 12, 2022

VictorVelarde Jan 11, 2022 •

edited

Loading

padawannn Jan 12, 2022

VictorVelarde Jan 12, 2022

padawannn Jan 12, 2022

VictorVelarde Jan 11, 2022

padawannn Jan 12, 2022

padawannn Jan 12, 2022

VictorVelarde Jan 11, 2022

padawannn Jan 12, 2022

VictorVelarde Jan 12, 2022

padawannn Jan 12, 2022

VictorVelarde left a comment

VictorVelarde commented Jan 18, 2022 •

edited

Loading

padawannn commented Jan 19, 2022 •

edited

Loading

VictorVelarde commented Jan 19, 2022

Josmorsot left a comment

Josmorsot Jan 28, 2022

padawannn Jan 28, 2022

bbecquet Jan 28, 2022

padawannn Jan 28, 2022

bbecquet left a comment

shortcut-integration bot commented Jan 28, 2022

VictorVelarde left a comment

		@@ -0,0 +1,30 @@
		import { EditableGeoJsonLayer } from '@nebula.gl/layers';

		const EVENT_TYPES = ['anyclick', 'pointermove', 'panstart', 'panmove', 'panend', 'keyup'];

		};

		// Set layers should be outside of the useEffect to avoid problems with layers that changes

Add FeatureSelection tool #271

Add FeatureSelection tool #271

Conversation

padawannn commented Jan 11, 2022 • edited by VictorVelarde Loading

coveralls commented Jan 11, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1778848646

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VictorVelarde Jan 11, 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

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

VictorVelarde left a comment

Choose a reason for hiding this comment

VictorVelarde commented Jan 18, 2022 • edited Loading

padawannn commented Jan 19, 2022 • edited Loading

VictorVelarde commented Jan 19, 2022

Josmorsot 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

bbecquet left a comment

Choose a reason for hiding this comment

shortcut-integration bot commented Jan 28, 2022

VictorVelarde left a comment

Choose a reason for hiding this comment

padawannn commented Jan 11, 2022 •

edited by VictorVelarde

Loading

coveralls commented Jan 11, 2022 •

edited

Loading

VictorVelarde Jan 11, 2022 •

edited

Loading

VictorVelarde commented Jan 18, 2022 •

edited

Loading

padawannn commented Jan 19, 2022 •

edited

Loading