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.

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

Allowing user-defined interaction handlers, and splitting them into a submodule #8886

Closed
arindam1993 opened this issue Oct 22, 2019 · 4 comments
Labels
api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API refactoring 🏗️

Comments

@arindam1993
Copy link
Contributor

arindam1993 commented Oct 22, 2019

Motivation

We have a number of issues in our issue queue that are regarding customizing various things regarding interactivity.

The idea with this proposal is to make this easier to do for developers by allowing them to bind their own event handlers declaratively at map init time, moving what we do in bind_handlers.js up and out so developers can integrate their own implementations and not necessarily have to rely on passing options and or activating/deactivating handlers that we have.

We can take this one step further, and extract our handlers and controls into separate npm packages ( similar to what babel and turf do ) so users not using our implementations don't have to pay the cost of the increased bundle size.

Design Alternative

The current status quo of adding additional options to Map, and or exposing configuration though map.{handlerName}.

This has the potential of adding a lot of cruft and complexity over-time as we try to accomodate additional requests.

Design

  • A newly constructed Map is interactive:false by default, and this option gets removed from the Map constructor.
  • Add a map.bindInteractionHandlers(handlers: EventHandlers) method, this is how you would add interactivity to the map.
type EventHandlers = {
    mousemove: function() {..},
    mousedown: function() {..},
    mouseup: function() {..},
    ....,
}
  • Separate out our interaction handlers to a separate sub-module, in order to enable the current state of interactive: true would look something like this.
import mapboxgl-interaction-handlers from '@mapbox/mapbox-gl-event handlers';
import mapboxgl from 'mapbox-gl-js';

// create an non-interactive map
const map = new Map(..);
// Add interactivity after the fact
map.bindInteractionHandlers(mapboxgl-interaction-handlers);

Pros:

  • Lower bundle size
  • Allows develops to fork our just interaction handlers and use them without having to fork entirety of gl-js if they need a different interaction paradigm.
  • Developers already using existing gesture processing libraries like hammer.js can have better integration with the rest of their app.

Cons:

  • API Breaking change :(
  • Requires some refactoring within gl-js, we currently query the interaction state globally on map for things like zooming hackily throughout the codebase.
  • Additional overhead on our side for maintaining an additional module and publishing and releasing it.
@arindam1993 arindam1993 added refactoring 🏗️ breaking change ⚠️ Requires a backwards-incompatible change to the API api 📝 labels Oct 22, 2019
@kkaefer
Copy link
Member

kkaefer commented Oct 23, 2019

if they need a different interaction paradigm

What would those interaction paradigms be? Could you provide any examples/use cases before adding an API?

@arindam1993
Copy link
Contributor Author

arindam1993 commented Oct 23, 2019

if they need a different interaction paradigm

What would those interaction paradigms be? Could you provide any examples/use cases before adding an API?

Some common configurations I can think of,

On Desktop:

  1. left-click pan, right-click pitch/rotate, scroll zoom (current)
  2. middle-click pan, right-click pitch/rotate, scroll zoom ( popular if you want to build an app that requires picking and selecting things )
  3. modifier+left-click pan, modifier+left-click pitch/rotate, scroll zoom (generally implemented as along with 2. to make it laptop trackpad friendly)
  4. 1 or 2 or 3 but with modifier+scroll for zooming so page scrolling doesn't get blocked.
  5. any of 1 -4 but reserve right-click for a custom context menu
  6. something with Pointerlock? (https://developer.mozilla.org/en-US/docs/Web/API/Pointer_Lock_API)

On mobile:

  1. single finger pan, multi finger pitch/rotate/zoom (current)
  2. multi-finger pan ( to allow scrolling )
  3. Maybe someone wants to building something with pen/stylus support 🤷‍♂ (https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent/pressure)

Right now our approach is kinda forceful, so I was thinking decoupling the logic. making it optional with a clear interface boundary might make it easier to implement all this variety.
This could also spawn a small set of community-maintained interaction handlers for some common use-cases.

@kkaefer
Copy link
Member

kkaefer commented Oct 24, 2019

Thanks; I think that makes sense.

@mourner
Copy link
Member

mourner commented Nov 4, 2019

I'm 👎 on cutting this out into a different repo. This will increase complexity and maintaining burden quite a bit, and I don't anticipate users plugging their own implementations all at once, since this is a lot of code — usually they would customize some things while leaving others at default behavior. 👍 on improving handler code extensibility though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API refactoring 🏗️
Projects
None yet
Development

No branches or pull requests

4 participants