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

Adds warning to geolocate control when unsupported #6923

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

aendra-rininsland
Copy link
Contributor

Launch Checklist

  • 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

This is a really basic pull request that adds a simple warning to the geolocate control when it's added to a map in a context that is unsupported.

Mainly submitting this because I couldn't figure out for the life of me why the control wasn't displaying, turns out I was serving the page over HTTP. Adding a warning informs the developer that this is indeed the expected behaviour.

@@ -259,7 +259,10 @@ class GeolocateControl extends Evented {
}

_setupUI(supported: boolean) {
if (supported === false) return;
if (supported === false) {
warnOnce('Geolocation not supported. The Mapbox geolocate control will not be rendered.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for adding this warning, since the developer has asked for the GeolocateControl, we should at least warn when that hasn't happened.

Could we change the warning to Geolocation support is not available, the GeolocateControl will not be visible.?

Copy link
Contributor Author

@aendra-rininsland aendra-rininsland Jul 9, 2018

Choose a reason for hiding this comment

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

@andrewharvey Absolutely, will update that now. Edit: Done!

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Thanks!

@jfirebaugh jfirebaugh merged commit b192262 into mapbox:master Jul 9, 2018
@aendra-rininsland aendra-rininsland deleted the patch-1 branch July 10, 2018 14:30
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