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

Check map maxBounds in Geolocate control #8756

Merged
merged 8 commits into from
Oct 12, 2019
Merged

Conversation

ThisIsOstad
Copy link
Contributor

@ThisIsOstad ThisIsOstad commented Sep 14, 2019

Launch Checklist

Changes:
The GeolocateControl with a maxBounds set on the map when a user is located outside of the maxBounds, keeps trying to move the camera outside of the maxBounds and causes a bad experience for users. In this PR, I checked map maxBounds on geolocate success callback and if the located position is out of maxBounds, remove location dot marker, update control classList, emit an event called outofmaxbounds and return to avoid camera update.

Related issue: #4872

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

@andrewharvey
Copy link
Collaborator

This won't entirely fix #4872, since the underlying issue there is requesting a flyTo outside the maxBounds is currently undefined behaviour, so that would need to be resolved to close #4872.

If we decide in #4872 to require the API user to make their own check, then in my opinion this proposed solution is the way to go. We make that check first internally in GeolocateControl, and then only flyTo if it's valid, and then emit an error back to the API user so they can decide what to do (like show an alert), exactly how you've done this PR.

In lieu of a decision on #4872, I think it's good to implement this PR even if just for the time being (although based on my comment at #4872 (comment), I do think this is probably the right approach).

Copy link
Collaborator

@andrewharvey andrewharvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me. I think emitting an event is the right way to handle this.

@andrewharvey
Copy link
Collaborator

cc @ryanhamley I'm happy with this change, are we good to merge or should I wait for someone from Mapbox to have a chance to review?

@ryanhamley
Copy link
Contributor

Looks good to me! Thanks @MoradiDavijani and @andrewharvey

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

Successfully merging this pull request may close these issues.

3 participants