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

[wip] add showUserHeading option to GeolocateControl #8929

Closed
wants to merge 3 commits into from

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Oct 31, 2019

Launch Checklist

  • briefly describe the changes in this PR

Add a showUserHeading option to the GeolocateControl to show the device heading as an arrow next to the user location dot.

Sensor access is disabled by default in Safari from iOS 12.2+, so for this to work you need to enable that setting in Safari Settings, once enabled it uses deviceOrientationEvent.webkitCompassHeading.

closes #8329
closes #8034

  • include before/after visuals or gifs if this PR includes visual changes

heading

  • 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 or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • cross device testing
  • investigate if it's better to use https://developer.mozilla.org/en-US/docs/Web/API/Coordinates/heading instead of deviceorientation.

@andrewharvey andrewharvey force-pushed the 8329-geolocate-heading-with-marker-bearing branch from e9199a9 to 4ab7d6e Compare October 31, 2019 03:17
@andrewharvey andrewharvey force-pushed the 8329-geolocate-heading-with-marker-bearing branch from 4ab7d6e to ab0d5c1 Compare November 2, 2019 01:49
@ahk
Copy link
Contributor

ahk commented Nov 7, 2019

Thanks for this PR. I'm curious if you have knowledge/experience/thoughts about how many users choose to enable sensor access in mobile Safari. Is this different from the permissions dialog that asks if location data should be enabled on a per-site basis?

@andrewharvey
Copy link
Collaborator Author

Thanks for this PR. I'm curious if you have knowledge/experience/thoughts about how many users choose to enable sensor access in mobile Safari. Is this different from the permissions dialog that asks if location data should be enabled on a per-site basis?

No idea how many enable it, but probably very little have it enabled since it's off by default and most users would have no reason to enable it, that said an application could prompt you to go into settings and enable, and I think applications can still do that independently if they like. So I still think we should support it when it is enabled.

It's an all or nothing switch in Safari (in the app settings) it's different to the permissions dialog for location access.

I investigated the coords.heading field from the geolocation API https://developer.mozilla.org/en-US/docs/Web/API/Coordinates/heading and found that iOS Safari doesn't support it (tested on iOS 12.4.2). Chrome on Android does, but that already supports an absolute bearing from deviceorientation.

@ahk ahk requested review from mourner and ahk November 8, 2019 22:43
@ahk
Copy link
Contributor

ahk commented Nov 8, 2019

I investigated the coords.heading field from the geolocation API

Do you think we should also support this field as well? I have Firefox and Firefox Preview on my Android phone. As well it's possible for desktop/laptop Firefox to have a compass reading in some weird cases right?

@ahankeosi
Copy link

Any movement on this? Would really love this feature!

@andrewharvey
Copy link
Collaborator Author

Any movement on this? Would really love this feature!

Nope, feel free to pick up my work in progress if you feel it's helpful.

@CptHolzschnauz
Copy link

Stick to one's guns ! Any movement to PR?

@andrewharvey
Copy link
Collaborator Author

this was completed by @tsuz in #10817

@andrewharvey andrewharvey deleted the 8329-geolocate-heading-with-marker-bearing branch July 9, 2021 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add heading to GeolocateControl Give Geolocation Control Marker Pitch Alignment Properties
4 participants