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

onViewportChange not triggered using NavigationControl #234

Closed
charlietfl opened this issue Jul 7, 2019 · 6 comments
Closed

onViewportChange not triggered using NavigationControl #234

charlietfl opened this issue Jul 7, 2019 · 6 comments
Assignees

Comments

@charlietfl
Copy link

charlietfl commented Jul 7, 2019

Describe the bug
Using default map zoom controls not triggering viewport change event in parent <MapGL>

To Reproduce
Steps to reproduce the behavior:

  1. Go to docs example https://urbica.github.io/react-map-gl/#/Controls/NavigationControl
  2. Edit code block adding onViewportChange={viewport=>console.log(JSON.stringify(viewport))} to <MapGL> component
  3. Open console, drag map center...see new viewport config in console
  4. Change zoom using +/- in NavControl ... no output

Expected behavior
Would expect event to be triggered using NavigationControl but instead it only seems to affect the map-gl instance

Desktop (please complete the following information):

  • OS: Windows
  • Browser: Chrome
@stepankuzmin stepankuzmin self-assigned this Jul 8, 2019
@jperals
Copy link

jperals commented Aug 26, 2019

I'd like to have this fixed too. @stepankuzmin Shall I submit a PR?

@stepankuzmin
Copy link
Member

Hey @jperals! It would be really great! PRs are welcome!

@jperals
Copy link

jperals commented Aug 26, 2019

It seems like the problem is that MapGL's _onViewportChange filters events that have an originalEvent property set, but it turns out that for certain interactions (among them, those coming from the NavigationControl) don't provide this property: mapbox/mapbox-gl-js#6405

I worked around this by just checking that the event exists (or that it's truthy, to be more exact) instead of its properties: jperals@f6fd40b

So this is the simplest solution I could come up with. Would it make more sense to check for some other properties in the object instead? This doesn't seem to have side effects for our use case and passes the tests (though I had to modify one of them as you can see), but of course I'm open to your feedback @stepankuzmin.

@stepankuzmin
Copy link
Member

Thanks for the research, @jperals!

Unfortunately we can't remove check for originalEvent because it is the only way to detect user-initiated events. The most concise way to fix this issue, IMO, is to add originalEvent to events initiated using NavigationControl.

I've created PR for this in mapbox-gl-js repo mapbox/mapbox-gl-js#8693

@stepankuzmin
Copy link
Member

This should be fixed in the nest version of mapbox-gl since PR is merged mapbox/mapbox-gl-js#8693

@derwaldgeist
Copy link

Just stumbled upon the same problem in the GeolocationControl (#276). Is there any update on when a new version will arrive that fixed this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants