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

Add geolocation control #2283

Closed
wants to merge 2 commits into from
Closed

Add geolocation control #2283

wants to merge 2 commits into from

Conversation

bhousel
Copy link
Contributor

@bhousel bhousel commented Mar 17, 2016

Not ready to merge yet. I want to

  • make it so that all the button controls can coexist happily.
    Nevermind: they already can coexist happily, I just didn't understand the addControl code.
  • there is probably a better pace for the navigator code to be, like js/util/browser?

(closes #1939)

screenshot 2016-03-17 18 45 23

@@ -9,7 +9,8 @@ var map = new mapboxgl.Map({
hash: true
});

map.addControl(new mapboxgl.Navigation());
// map.addControl(new mapboxgl.Navigation());
map.addControl(new mapboxgl.Geolocate());
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #2281, I would like to see this as a seperate controls.html page.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is actually useful even on a simple debug page. Maybe I would even consider adding it by default in GL JS maps if it's integrated into the zoom buttons toolbar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per #1929 / #1554 I am going to make the button layout more flexible. This will involve splitting up the Navigation control and moving some of what it does into a container class to manage the screen regions. Developers should be able to pick and choose which navigation controls they want to appear on the map, and this geolocation button is no different.

Copy link
Member

Choose a reason for hiding this comment

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

@bhousel Sounds great!

@lucaswoj
Copy link
Contributor

So cool! I can't wait to merge 😄

@bhousel
Copy link
Contributor Author

bhousel commented Mar 22, 2016

I think this is ok to merge.
I added a check for navigator.geolocation to js/util/browser.js. This is mostly just for node.js, since geolocation is supported on all the browsers that we support.

I also left the control enabled in debug/site.js. If it goes away as part of a separate effort to revamp our debug page, thats cool too.

One more 👀 @lucaswoj ?

@lucaswoj
Copy link
Contributor

LGTM! 🚢

@bhousel
Copy link
Contributor Author

bhousel commented Mar 22, 2016

thanks, merged as 196a235, 15ba191

@bhousel bhousel closed this Mar 22, 2016
@bhousel bhousel deleted the geolocation-control branch March 22, 2016 21:09
@andreipopovici
Copy link

Pinging with #3473 for some much-appreciated improvement on mobile devices. It's an easy win!

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.

Add a "jump to current location" control
4 participants