-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
Add FeatureSelection tool #271
Changes from 1 commit
aa1e751
2ac4f1d
e340c32
5f55f19
04f1f9f
224154a
6ee1a40
e73fd6f
74b97fd
cbafc72
0b13376
3052490
1cef257
d745cc0
f0bbb9a
aafbac8
cd4c0ef
2f5354e
fab0cf7
30e7c45
70056a0
1c7484f
2f414d1
396562c
a5cfaab
e7be25b
af39813
52b48a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import { Credentials } from '@carto/react-api'; | ||
import { OauthApp } from '@carto/react-auth'; | ||
import { CartoBasemapsNames } from '@carto/react-basemaps'; | ||
import { DRAW_MODES, Viewport } from '@carto/react-core'; | ||
import { Viewport } from '@carto/react-core'; | ||
import { Geometry } from 'geojson'; | ||
|
||
export type ViewState = { | ||
|
@@ -45,7 +45,7 @@ export type CartoState = { | |
spatialFilter: Geometry, | ||
featuresReady: { [key: string]: boolean }, | ||
drawingToolEnabled: boolean, | ||
drawingToolMode: typeof DRAW_MODES | ||
drawingToolMode: string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why such change here? isn't it better to use a more restricted type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because |
||
} & InitialCartoState; | ||
|
||
export type InitialOauthState = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { EditableGeoJsonLayer } from '@nebula.gl/layers'; | ||
|
||
const EVENT_TYPES = ['anyclick', 'pointermove', 'panstart', 'panmove', 'panend', 'keyup']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, I'm going to add a link to the original code and write a small explanation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
export default class EditableCartoGeoJsonLayer extends EditableGeoJsonLayer { | ||
_addEventHandlers() { | ||
const eventManager = this._getEventManager(); | ||
const { eventHandler } = this.state._editableLayerState; | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is 100 the "top priority" or how does is work? I mean, is it an arbitrary number?. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't know XD. This code has been copied from the original nebula code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. XD, ok then add this link to code... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
}); | ||
} | ||
} | ||
|
||
_removeEventHandlers() { | ||
const eventManager = this._getEventManager(); | ||
const { eventHandler } = this.state._editableLayerState; | ||
|
||
for (const eventType of EVENT_TYPES) { | ||
eventManager.off(eventType, eventHandler); | ||
} | ||
} | ||
|
||
_getEventManager() { | ||
return this.props.eventManager || this.context.deck.eventManager; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectDrawingToolMode
is a selector that returnfalse
if thespatialFilter
is null in the store (the default state). I think that creating another enum type could be over-engineeringThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just see there are other options without adding boolean to the mix:
"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
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done the
b
option