Add drag and drop support for Marker #576

schnerd · 2018-08-11T16:57:24Z

Resolves issue #483

This PR adds support for drag and drop events to the <Marker> component.

Simply add a draggable prop to your Marker, and subscribe to whatever events you need:

<Marker 
  longitude={marker.longitude}
  latitude={marker.latitude}
  draggable
  onDragStart={this._onMarkerDragStart}
  onDrag={this._onMarkerDrag}
  onDragEnd={this._onMarkerDragEnd}
>
  <Pin size={20} />
</Marker>

This API design is intended to be similar to Mapbox's draggable markers.

schnerd · 2018-08-11T16:58:26Z

examples/draggable-markers/package.json

+    "webpack": "^2.4.0",
+    "webpack-dev-server": "^2.4.0"
+  }
+}


We should eventually look into not having all this boilerplate for each example.

We should eventually look into not having all this boilerplate for each example.

It is our goal that each example folder can be independently copied out of this repo and work stand-alone, so we do want a minimal set of "boilerplate".

That said, we really want that boilerplate/configuration to be as minimal as possible.

Unfortunately the current fashion in javascript bundlers and compilers is to over-engineer for modularity and they do not seem to offer umbrella modules (not a single unneeded development plugin must installed, even if that requires the user to add 20 lines to package.json and has no effect the final bundle size :).

Two options here are probably

move to babel-preset-env which should remove two lines

explore create-react-app, which now seems to be supported by mapbox-gl

schnerd · 2018-08-11T16:59:35Z

src/components/base-control.js

+    if (eventManager && this._events) {
+      eventManager.off(this._events);
+    }
+  }


@Pessimistress Seemed like it made sense to remove events if the control is unmounted – let me know if I'm missing something.

schnerd · 2018-08-11T17:02:23Z

src/components/marker.js

+      pancancel: this._onDragCancel.bind(this)
+    };
+    eventManager.on(this._dragEvents);
+  }


These events need to be bound to the map itself (instead of the marker), otherwise you get behavior like this when the mouse moves outside the pin:

And it makes sense to attach these events after panstart, otherwise with 1000 markers you'd have 1000 onDrag event handlers being invoked after each panmove.

schnerd · 2018-08-11T17:03:58Z

test/src/components/marker.spec.js

+  result.unmount();
+
+  t.end();
+});


Was going to write more tests for draggable functionality specifically, but it seems like react test renderer doesn't really do refs, making it difficult to test the desired behavior.

schnerd · 2018-08-11T17:05:11Z

examples/main/stylesheets/main.scss

@@ -245,9 +245,6 @@ hr.short {
  bottom: 24px;
  color: $white-40;
 }
-.overlays {
-  cursor: crosshair;
-}


Having crosshair as the cursor across the examples & website seemed like a mistake. It overrides the default cursor behavior of the library (and doesn't look great IMO)

schnerd · 2018-08-11T17:06:12Z

src/components/marker.js

@@ -31,7 +31,12 @@ const propTypes = Object.assign({}, BaseControl.propTypes, {
  // Offset from the left


I'm not in love with how much code ended up being required in here, but I still think it's a manageable size and broken down into small methods.

I'm not in love with how much code ended up being required in here, but I still think it's a manageable size and broken down into small methods.

How generic is this code? If another component wanted to also implement dragging would it be highly reusable. Could it make sense to organize it as

Marker > DraggableControl > BaseControl

It could look more appealing to have these methods in a separate class that is focused on dragging, rather than mixed with the Marker specific functionality.

I like this idea, let me try restructuring it that way.

ibgreen · 2018-08-13T14:44:10Z

docs/components/marker.md

+- `event` - The pointer event.
+  + `event.lngLat` - The geo coordinates of the drag event, as `[lng, lat]`.
+
+##### `onDragEnd` {Function}


Is there a concept of DragCancel? (ESCAPE key etc)

So technically there is a pancancel event in hammer which could theoretically warrant an onDragCancel call, but I couldn't figure out how to trigger it (escape, dragging outside viewport, etc didn't do anything).

We could add support for escape key, though that would require another explicit keyup handler. If we're mostly trying to stay aligned with mapbox draggable markers, they currently don't have a cancel event so it might be fine to omit for now.

ibgreen · 2018-08-13T14:53:47Z

examples/draggable-markers/package.json

+    "webpack": "^2.4.0",
+    "webpack-dev-server": "^2.4.0"
+  }
+}


We should eventually look into not having all this boilerplate for each example.

It is our goal that each example folder can be independently copied out of this repo and work stand-alone, so we do want a minimal set of "boilerplate".

That said, we really want that boilerplate/configuration to be as minimal as possible.

Unfortunately the current fashion in javascript bundlers and compilers is to over-engineer for modularity and they do not seem to offer umbrella modules (not a single unneeded development plugin must installed, even if that requires the user to add 20 lines to package.json and has no effect the final bundle size :).

Two options here are probably

move to babel-preset-env which should remove two lines

explore create-react-app, which now seems to be supported by mapbox-gl

ibgreen · 2018-08-13T14:54:57Z

examples/draggable-markers/webpack.config.js

+    }]
+  },
+
+  resolve: {


I think this resolve is no longer needed and could be removed from examples

ibgreen · 2018-08-13T14:59:24Z

src/components/marker.js

@@ -31,7 +31,12 @@ const propTypes = Object.assign({}, BaseControl.propTypes, {
  // Offset from the left


I'm not in love with how much code ended up being required in here, but I still think it's a manageable size and broken down into small methods.

How generic is this code? If another component wanted to also implement dragging would it be highly reusable. Could it make sense to organize it as

Marker > DraggableControl > BaseControl

It could look more appealing to have these methods in a separate class that is focused on dragging, rather than mixed with the Marker specific functionality.

Add drag and drop support for Marker

513fd66

schnerd requested review from Pessimistress and ibgreen August 11, 2018 16:57

schnerd commented Aug 11, 2018

View reviewed changes

ibgreen approved these changes Aug 13, 2018

View reviewed changes

schnerd added 2 commits August 13, 2018 10:30

Refactor drag logic into DraggableControl

1f92f6a

fix draggable-markers example

a31d559

Pessimistress approved these changes Aug 13, 2018

View reviewed changes

schnerd merged commit 9c302ae into master Aug 13, 2018

Pessimistress deleted the draggable-markers branch October 27, 2018 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add drag and drop support for Marker #576

Add drag and drop support for Marker #576

schnerd commented Aug 11, 2018

schnerd Aug 11, 2018

ibgreen Aug 13, 2018

schnerd Aug 11, 2018

schnerd Aug 11, 2018

schnerd Aug 11, 2018 •

edited

Loading

schnerd Aug 11, 2018 •

edited

Loading

schnerd Aug 11, 2018

ibgreen Aug 13, 2018

schnerd Aug 13, 2018

ibgreen Aug 13, 2018

schnerd Aug 13, 2018 •

edited

Loading

ibgreen Aug 13, 2018

ibgreen Aug 13, 2018

schnerd Aug 13, 2018

ibgreen Aug 13, 2018

		@@ -31,7 +31,12 @@ const propTypes = Object.assign({}, BaseControl.propTypes, {
		// Offset from the left

Add drag and drop support for Marker #576

Add drag and drop support for Marker #576

Conversation

schnerd commented Aug 11, 2018

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

schnerd Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

schnerd Aug 11, 2018 • 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

schnerd Aug 13, 2018 • 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

schnerd Aug 11, 2018 •

edited

Loading

schnerd Aug 11, 2018 •

edited

Loading

schnerd Aug 13, 2018 •

edited

Loading