visgl / react-map-gl

React friendly API wrapper around MapboxGL JS
http://visgl.github.io/react-map-gl/
Other
7.83k stars 1.35k forks source link

Click event isn't fired on the original map #569

Closed vkammerer closed 5 years ago

vkammerer commented 6 years ago

Since https://github.com/uber/react-map-gl/pull/565 landed in 3.3.4, the original map layer doesn't capture clicks anymore

const map = this.mapRef.getMap();
map.on('click', e => {
  console.log(e);
  // nothing gets logged
});

This makes it impossible to use certain features, such as handling clicks on clusters for example.

Is there a way to get the clicks to be passed to the original map layer?

Pessimistress commented 6 years ago

Why do you need to register your own event listener? Does the onClick prop not work?

vkammerer commented 6 years ago

My use case is that I want to be able to handle clicks on clusters, but I guess there could be many others..

Pessimistress commented 6 years ago

This module is a React wrapper for mapbox-gl so you are encouraged to use the React API wherever possible. There is no guarantee for any mapbox-gl native examples to work.

In your use case, please use the onClick prop to access the features under the pointer.

vkammerer commented 6 years ago

There is no guarantee for any mapbox-gl native examples to work

Ok wow, this is a very strong statement. Thank you for the clarification. I believe this will be a major blocker for many teams, as it means that once you opt in for this library, you cannot benefit from the original API.

This should definitely be stated at the top of the README to warn users.

Pessimistress commented 6 years ago

To be clear, here is how you can capture a click event and recreate that example:

_onClick(event) {
  // either
  const feature = event.features.find(f => f.layer.id === 'clusters');
  // or
  const point = [event.center.x, event.center.y];
  const feature = this.mapRef.queryRenderedFeatures(point, { layers: ['clusters'] })[0];

  if (feature) {
    // look up cluster expansion zoom
  }
}

render() {
  return <MapGL onClick={this._onClick} ... />;
}

this is a very strong statement.

Not really. Front and foremost, the goal of this library is to provide an interface that conforms with the React programming conventions. Similarly, you cannot go find a vanilla HTML/JavaScript code snippet and expect it to work as-is with React.

In this specific case, mapbox-gl's native event API does not support itself being used as a stateless component, forcing us to hijack its event handling system. It is indeed not an idea situation. We are in conversation with Mapbox but their ETA is unclear. I agree with you that the documentation can be more clear about breaking the native event handling, and I will make sure it is reflected in the docs.

I hope you can give the above solution a try.

vkammerer commented 6 years ago

Thank you very much for your reply and the code sample. Your method will work well with my application and I'm happy to remove the quick hack I had implemented:

.overlays {
  pointer-events: none !important;
}

I certainly don't understand the whole rationale behind the decision to hijack Mapbox's events, but I am surprised by your argument on the lack of importance of ensuring that the Mapbox API is still fully operational..

From my experience working with another react layer on top of webgl (react-three-renderer), a complex application will use both APIs:

  1. the React layer for:
    • the many benefits of the declarative approach
    • the consistency with the rest of the application
  2. and also sometimes the original underlying imperative API for:
    • specific effects that the react lib has not implemented yet
    • performance micro optimizations
    • or simply to copy paste an example among the many provided by the original API's documentation (and Mapbox's documentation is great in that regard)

So in my opinion, while a user of this library should of course be encouraged to use the react layer as much as possible, it is also very important to ensure that the original mapbox gl API is fully functional.

But appart from this: thanks a lot for the example :) this will help me and was the solution I was looking for.

Pessimistress commented 6 years ago

I certainly don't understand the whole rationale behind the decision to hijack Mapbox's events

The primary reason is that Mapbox's move event is fired after it updates, while in the React context, any event-triggered rerender is propagated to all components in the next animation frame. This causes controls/overlays to be always one frame behind the base map during user interaction or viewport animation.

This would not be an issue if you use Mapbox's native controls. However, because of how Mapbox's controls API is designed (particularly the usage of innerHTML) we have to re-implement all control component in React instead of wrapping their native counterparts.

In short: you can't use any React controls or overlays with the base map, while using the native event handling. You are not the first or last person who wishes to leverage the native events more; and trust me it's not fun to maintain our own event handling system to parity. I have a pending request on the mapbox-gl-js repo.

vkammerer commented 6 years ago

Thank you very much for the detailed reply. I am trying to stay within the library's boundaries so far and I haven't encountered any real blocker.

vkammerer commented 6 years ago

Hi @Pessimistress, thank you again for the previous response. I have followed your advice and used the onClick prop on InteractiveMap to handle events.

However, I have noticed that there is a 300ms delay between the moment a user clicks and the moment the onClick handler is actually called. I digged a bit in the library and understand that https://github.com/uber-web/mjolnir.js is used, and that the 300ms is necessary to handle a doubleClick event separately from the click event.

But is there still a way to get rid of the 300ms delay, and to cancel the upcoming doubleClick on certain conditions?

This would make my current map much more responsive than what it is today.

raja commented 6 years ago

Also looking to remove the 300ms delay.

Pessimistress commented 6 years ago

@vkammerer @raja Do you wish to completely remove the double click to zoom functionality, or have the click event fired even when double clicking?

If the former, I could make a change so that if doubleClickZoom is set to false, the 300ms delay is removed.

vkammerer commented 6 years ago

@Pessimistress thank you for the reply.

In my opinion, it would be best if:

The use case I have, and which I guess people willing to use this have, is that I want to be able to:

The way I see it, I would:

dabrowski-adam commented 6 years ago

@Pessimistress

I have a relevant issue so I'm posting here—however reading the discussion has only made me more confused. 🙃

I'm on 3.3.4 and when I pass doubleClickZoom={false} to <ReactMapGL/> the click delay is removed. However, once I insert <NavigationControl/> it returns.

Pessimistress commented 6 years ago

@dabrowski-adam Good catch. That is a bug.

Pessimistress commented 6 years ago

@vkammerer In your use case, if the user double clicked on a feature, do they both select it AND zoom? Seems to me that you actually want a way to cancel the double click gesture if the first click is on a feature?

vkammerer commented 6 years ago

@Pessimistress Yes indeed, that would be ideal.

But from my understanding reading the code, it seems like this would require much more effort than just adding another click handler. My hypothesis is that users who double click to zoom will mostly do it outside of pins / markers, so I could definitely with a partial first implementation.

But if you think it is possible (within your time :) ) to cancel a double click, then that's great. Maybe this handler should then return a boolean indicating if the next click should be a part of a double click?

onImmediateClick(event) {
  const feature = event.features.find(f => f.layer.id === 'clusters');
  if (!feature) return true;
  // Otherwise do the things I want to do with a click on my feature and then:
  return false;
}
Pessimistress commented 6 years ago

@vkammerer We use hammerjs' requireFailure in the current implementation. I can poke around and see if it's possible to drop it in the middle of a gesture.

cyrilchapon commented 5 years ago

What is the status of this as of february 2019 ?

We're using

"react-map-gl": "^4.0.10"

And we've been trying doubleClickZoom to false without success to remove the delay. We don't have any overlay

vkammerer commented 5 years ago

@Pessimistress it looks like a decision was made on mapbox side regarding these events to handle click and dblclick separately: https://github.com/mapbox/mapbox-gl-js/issues/6524

Should the implementation in this repository follow the same spec? This would remove the 300ms delay, which currently degrades the user experience.

I have unfortunately resorted to reenable my previous hack to remove it for now:

.overlays {
  pointer-events: none !important;
}
Pessimistress commented 5 years ago

@cyrilchapon @vkammerer this seems to be a regression due to https://github.com/uber/react-map-gl/pull/626. I'll put up a patch with the following behavior:

Regarding Mapbox's click behavior: changing onClick will likely break existing applications. I can add an additional callback that does this. Any suggestion on the prop name? onNativeClick?

vkammerer commented 5 years ago

onNativeClick sounds ok to me!

cyrilchapon commented 5 years ago

Just thinking out loud.. Wouldn't it be interesting to expose this delay roughly as a prop.. ?

doubleClickTriggerDelay

Pessimistress commented 5 years ago

@cyrilchapon @vkammerer onClick fix and onNativeClick addition is published in 4.0.12.

We can potentially expose the double click delay but that will require more changes to the event manager.

vkammerer commented 5 years ago

Thanks a lot @Pessimistress !

cyrilchapon commented 5 years ago

WoW thanks for reactivity ☺️ Test and feed-back incoming

macobo commented 5 years ago

Thanks for adding the native handler! I wonder if there's a more elegant solution here though.

Pessimistress commented 5 years ago

@macobo I tried to avoid making a breaking change. When we go to the next major release we'll definitely want to revisit the API design.

PelumiWeb commented 3 years ago

This is all good for react-native, but there is no onClick props for react-map-gl, please does anyone knows how to pass an event listener on the layers with react-map-gl. or is it not possible to do so?

ashnewport commented 2 years ago

@PelumiWeb I just added a working solution to click a marker on a layer and display related information in a popup over at https://github.com/visgl/react-map-gl/issues/1059#issuecomment-969901109.

someError commented 2 years ago

Thank you very much for your reply and the code sample. Your method will work well with my application and I'm happy to remove the quick hack I had implemented:

.overlays {
  pointer-events: none !important;
}

I certainly don't understand the whole rationale behind the decision to hijack Mapbox's events, but I am surprised by your argument on the lack of importance of ensuring that the Mapbox API is still fully operational..

From my experience working with another react layer on top of webgl (react-three-renderer), a complex application will use both APIs:

  1. the React layer for:
  • the many benefits of the declarative approach
  • the consistency with the rest of the application
  1. and also sometimes the original underlying imperative API for:
  • specific effects that the react lib has not implemented yet
  • performance micro optimizations
  • or simply to copy paste an example among the many provided by the original API's documentation (and Mapbox's documentation is great in that regard)

So in my opinion, while a user of this library should of course be encouraged to use the react layer as much as possible, it is also very important to ensure that the original mapbox gl API is fully functional.

But appart from this: thanks a lot for the example :) this will help me and was the solution I was looking for.

you saved my night dude