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

Geolocate watch position #3739

Merged
merged 2 commits into from
Dec 6, 2016

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Dec 5, 2016

Not sure if this is a feature which is wanted or not. I saw #3008 which seems to rule out adding the a dot on the map for the users location, but people still use GL JS for mobile and native support to track a location is useful.

  • briefly describe the changes in this PR
    Added on option to the Geolocate control to watch the position (ie. reposition the map as the user moves) using Geolocation watchPosition.

  • write tests for all new functionality

  • document any changes to public APIs

  • post benchmark scores

  • manually test the debug page
    Tested in Chrome and Firefox on Desktop and Mobile. (ps in Chrome Dev Tools, ... -> More Tools -> Sensors comes in handy)

I based this off #3738

TODO

  • use a blue for the geolocate toggle which stands out more

@lucaswoj
Copy link
Contributor

lucaswoj commented Dec 5, 2016

Not sure if this is a feature which is wanted or not. I saw #3008 which seems to rule out adding the a dot on the map for the users location, but people still use GL JS for mobile and native support to track a location is useful.

Apologies. My language in #3008 reflects a position a little stronger than what I currently feel. While implementing this feature wasn't a priority for the GL team, we're happy to accept a PR that implements it.

@lucaswoj
Copy link
Contributor

lucaswoj commented Dec 5, 2016

For other reviewers' reference, this is what the "watching" and "not watching" icons look like.

screen shot 2016-12-05 at 12 02 53 pm

screen shot 2016-12-05 at 12 02 20 pm

use a blue for the geolocate toggle which stands out more

I think the current design looks good. Any thoughts @tristen?


// toggle watching the device location
if (this.options.watchPosition) {
if (this._watching) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to eliminate the _watching flag and use _geolocationWatchID as the single source of truth about which state we're in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I've just changed it. The only caveat was that 0 seems to be a valid id (spec doesn't rule it out and Firefox Desktop seems to start at 0) so I need to explicitly check !== undefined.

@andrewharvey andrewharvey force-pushed the geolocate-watch-position branch from 5be5921 to a906f8e Compare December 5, 2016 22:23
@andrewharvey
Copy link
Collaborator Author

I've just added aria-pressed support.

I think the current design looks good. Any thoughts @tristen?

I think it works well too, it was more that when I was testing on mobile with the screen brightness turned down at night it was harder to distinguish between the two states.

The icon is duplicated in the stylesheet, I originally wanted to have just the black icon and use a CSS filter like this to change the color on toggle, but IE support for filter isn't present.

@tristen
Copy link
Member

tristen commented Dec 5, 2016

The blue is nice ... it's a shame we can't just change the fill color as a property value and I can see how it would be harder to distinguish between the two states.

I think we should represent this as a disabled state by dropping the opacity of the icon instead? That would save us from adding an encoded string in the stylesheet and make state more evident on dimly lit screens.

@andrewharvey
Copy link
Collaborator Author

I think we should represent this as a disabled state by dropping the opacity of the icon instead? That would save us from adding an encoded string in the stylesheet and make state more evident on dimly lit screens.

So there are two modes for this control. Is this what you're suggesting @tristen ?

  1. watchPosition == false
    on page load
    selection_426

user clicks the button which takes the map to their location, the button has no state.

  1. watchPosition == true
    on page load
    selection_425
    default state is off, user then toggles the button on to track location so state changes to on and becomes
    selection_426

I like the blue, it indicates that the control is actively watching the position. I think the one with opacity makes it look like the control is disabled and can't be clicked.

What about a tad lighter blue?

@tristen
Copy link
Member

tristen commented Dec 6, 2016

@andrewharvey I didn't fully read the proposed change. Right: dropping opacity would make this look disabled which we shouldn't do. No strong opinions here - I think a tad lighter blue makes sense.


This might be out of the realm of this PR but we should add these icon strings as url encoded svg like the others.

@andrewharvey andrewharvey force-pushed the geolocate-watch-position branch from a906f8e to 474aaa8 Compare December 6, 2016 01:23
@andrewharvey
Copy link
Collaborator Author

@andrewharvey I didn't fully read the proposed change. Right: An opacity change would make this look disabled which we shouldn't do. No strong opinions here - I think a tad lighter blue makes sense.

@tristen No problem. I've changed the blue to #00f. Not sure if that's too bright now though.

selection_427

This might be out of the realm of this PR but we should add these icon strings as url encoded svg like the others.

I agree, I've changed it over so they are all consistent and limits this premature optimization (at least you can change fill color in place).

Copy link
Contributor

@lucaswoj lucaswoj left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you so much @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