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 locate me #171

Merged
merged 3 commits into from
Oct 12, 2015
Merged

Add locate me #171

merged 3 commits into from
Oct 12, 2015

Conversation

jonahadkins
Copy link
Contributor

this adds enhancement #144 for a locate me feature

this adds enhancement #144 for a locate me feature
@jonahadkins
Copy link
Contributor Author

my first time installing node, grunt, etc - contribution instructions were great! 💯

locate feature references locate cdn with top left location - will need to add to css file for custom location.

@mapsam
Copy link
Member

mapsam commented Sep 13, 2015

Sweeeeeeet! Thanks for doing this 💃 I'm working on a big overhaul in the alpha branch right now, which might mean the leaflet-locatecontrol will be installed differently at some point (via npm instead of CDN), but hot diggity glad you were able to add it!

Not sure why the checks are failing here, though.

@mapsam mapsam mentioned this pull request Sep 13, 2015
37 tasks
@alukach
Copy link
Member

alukach commented Sep 13, 2015

Will this add the location as a layer or only zoom to a persons location? It looks like just the latter? I think both are valuable, so thanks for putting this in!

@mapsam
Copy link
Member

mapsam commented Sep 13, 2015

Ohhh location as a layer would be awesome.

@jonahadkins
Copy link
Contributor Author

Just zooms to the persons location 😞 - the users location can probably be obtained from the marker or centroid extent, but that's beyond my skills (for now).

i wasn't sure where to add the library to the project - there are a few css properties we can use to move the locate me button underneath the zoom in/out control.

@mapsam
Copy link
Member

mapsam commented Sep 13, 2015

No sad faces! This is all 😄s. I've noted "turn user location into layer" as a bonus feature in the alpha and it can be unrelated to this PR.

As for adding libraries, we've gone back and forth between using CDNs, npm & bundle, and require() in our files. I'm leaning towards npm & bundle because it lands in the middle of efficient and manual. Here's what it looks like in the alpha branch right now after you npm install XXXXXXX --save and bundle it.

@jonahadkins
Copy link
Contributor Author

ill wire it up to use npm & bundle 👍

@mapsam
Copy link
Member

mapsam commented Sep 14, 2015

Cool!

@mapsam
Copy link
Member

mapsam commented Sep 14, 2015

Hey @jonahadkins it looks like there is a relatively quick way to do this using the map.locate() function in mapbox.js. https://www.mapbox.com/mapbox.js/example/v1.0.0/geolocation/ - might not need to introduce another library! This also returns geometry, which could be used subsequently to make a new layer.

@jonahadkins
Copy link
Contributor Author

Even better - Ill work on that then

@jonahadkins
Copy link
Contributor Author

currently having beginner issues with this._map.locate(); throwing a Uncaught TypeError: Cannot read property 'locate' of undefined error

@nickpeihl
Copy link
Member

@jonahadkins I wonder if the this variable isn't available in the function due to closure issues? Unfortunately, I'm not sure what the best way to handle this is. Maybe @mapsam or @alukach have ideas.

@alukach
Copy link
Member

alukach commented Sep 15, 2015

@jonahadkins Haven't had a time to actually checkout your code, but it sounds likes your this object isn't what you think it is in that function. It's an awkward JS issue that is can bit strange if you haven't experienced it before. The error you posted is basically saying that this._map is undefined. Essentially, your this within the function (e) that you're defining refers to the scope of that function (not the whole class), so its this won't be the same object as the this on the lines above. There are two common ways to fix this:

  • Create a pointer to this outside of the function that won't be overwritten when you enter the function:
if (!navigator.geolocation) {
    geolocate.innerHTML = 'Geolocation is not available';
} else {
    var _this = this;  // Assigning the old 'this' to a variable that won't be overwritten
    geolocate.onclick = function (e) {
        e.preventDefault();
        e.stopPropagation();
        _this._map.locate();  // Explicitely using the old 'this'
    };
}
  • Use bind to tell the function to use the class' this:
if (!navigator.geolocation) {
    geolocate.innerHTML = 'Geolocation is not available';
} else {
    geolocate.onclick = function (e) {
        e.preventDefault();
        e.stopPropagation();
        this._map.locate();
    }.bind(this);  // Tells the function to use the provided 'this', not its own 'this'
}

I'm no expert, but I feel like either of those should help. I think option 2 is a bit cleaner.

Ps. Watch out for the indentation in that class. It's functionally the same, but a well indented chunk of code is much easier to read.

@mapsam
Copy link
Member

mapsam commented Sep 15, 2015

Yes, thanks @alukach! The confusion of this is mostly a relic in the alpha branch, which makes jumping into the code a bit more straightforward.

It will look more like this in alpha, with dc being short for dropchop:

var myLocation = dc.map.m.locate() // can use this anywhere, without referring to "this"

@jonahadkins
Copy link
Contributor Author

omg y'all IT WORKS

i do get this warning from chrome dev tools:

getCurrentPosition() and watchPosition() are deprecated on insecure origins, and support will be removed in the future. You should consider switching your application to a secure origin, such as HTTPS. See https://goo.gl/rStTGz for more details.

but IT WORKS

@mapsam
Copy link
Member

mapsam commented Sep 15, 2015

aw paying money for https sounds like a fun way to spend my day

@alukach
Copy link
Member

alukach commented Sep 15, 2015

So, allegedly you can set up SSL w/ custom domains for free: https://sheharyar.me/blog/free-ssl-for-github-pages-with-custom-domains/. Will add an issue. See #174

@mapsam
Copy link
Member

mapsam commented Sep 15, 2015

Whoa. Nice!

@jonahadkins
Copy link
Contributor Author

FYI - Everything Works - I think the button might need some JS help to stay put (the previous library we used did)
What else can I do / need to do on this branch?

@mapsam mapsam merged commit c61ef32 into cugos:master Oct 12, 2015
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.

4 participants