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 error checking to geocodeStringAndPan #220

Merged

Conversation

nstjean
Copy link
Contributor

@nstjean nstjean commented Dec 12, 2019

Fixes #214

Prevent hanging when user uses the location autocomplete search if the call returns an error.

Trying this PR again, last branch I think was just too mixed up.

map.setView([geometry.lat, geometry.lng], options.zoom);
onComplete = onComplete || function onComplete(response) {
if(response.status === "OK") {
let geometry = response.results[0].geometry.location;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what's causing the tests to fail. I've looked for any other instance that is calling this function and why it might be throwing errors but I can't see anything. It's only the tests, for some reason this is causing the tests to not build the BlurredLocation object.

@nstjean
Copy link
Contributor Author

nstjean commented Dec 12, 2019

Fixed the testing errors by removing the offending line, it did not want me referring the the results in a variable. Using response.results[0].geometry.location every time works.

@cesswairimu
Copy link
Contributor

Hi @nstjean, sorry just seeing this just now...rebase should fix this.
on your working branch locally do
git rebase -i main (here delete all the commits that were not made by you if any)

@nstjean
Copy link
Contributor Author

nstjean commented Dec 16, 2019

Ok, I'll try running that and see what I get!

@nstjean
Copy link
Contributor Author

nstjean commented Dec 16, 2019

I think I did it!! I don't know what the rebase did to dist/Leaflet.BlurredLocation.js though. Should I run grunt build and make a new commit with the updated version?

@jywarren
Copy link
Member

Ah, hm, it still seems out of date, but GitHub is indicating it can resolve the conflict so it must be pretty simple.

image

Does this have the /dist/ changes in it, so if we press "update branch" it should be merge-able? Thanks!

@nstjean
Copy link
Contributor Author

nstjean commented Dec 16, 2019

Ok updated /dist/

package.json Outdated
@@ -35,6 +35,7 @@
"resig-class": "^1.0.0"
},
"dependencies": {
"grunt-cli": "^1.3.2",
Copy link
Member

Choose a reason for hiding this comment

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

Ah, i think this may need to be in "dev-dependencies" so that it only gets included for developers, not for deployment? not a big deal but good practice. Thank you! https://docs.npmjs.com/specifying-dependencies-and-devdependencies-in-a-package-json-file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's interesting. I installed it and didn't realize it would get added in there. I'll move it to dev-dependencies!

@nstjean
Copy link
Contributor Author

nstjean commented Dec 17, 2019

@jywarren This should be all set! Hopefully!

Copy link
Collaborator

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Errors are handled nicely here :) Thanks!


map.setView([geometry.lat, geometry.lng], options.zoom);
onComplete = onComplete || function onComplete(response) {
if(response.status === "OK") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @nstjean , awesome!
Do we have response.code === 200? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you mean in addition to the response.status from google we should also check the http response code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes usually i have seen people checking code but i guess status also works fine!

@sagarpreet-chadha
Copy link
Collaborator

🎉
@jywarren it looks good to me! Thanks!

@sagarpreet-chadha sagarpreet-chadha merged commit 3b1f71b into publiclab:main Dec 20, 2019
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.

Location search not working
4 participants