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 a hook to get longitude and latitude #15

Merged

Conversation

fernandoamz
Copy link
Contributor

Implements

Add a new hook to get the geolocalisation from the browser, the hook will get the permissions and it will return the values.

@sairajchouhan
Copy link

Yea assign me I fill send a PR

@aromalanil
Copy link
Owner

@fernandoamz Please add the description, parameters and return value as per JSdoc guidelines.

@fernandoamz
Copy link
Contributor Author

@aromalanil, I send the correction

@aromalanil
Copy link
Owner

@fernandoamz Check out other suggestions also and remove the space between comment and the function.

@aromalanil
Copy link
Owner

aromalanil commented Oct 1, 2020

@fernandoamz I hope you can see the changes I have requested. If you can't I will mention it here.

  • An array is not needed for single return value so you can use
return latLong
  • Use meaningfull variable name, I would suggest using geoLocation instead of latLong.

  • A more elegent way to define object would be

let userPosition = {
      latitude = position.coords.latitude; 
      longitude = position.coords.longitude;
};       
  • The boolean geolocation passed to the function is unnecessary.

@fernandoamz
Copy link
Contributor Author

fernandoamz commented Oct 1, 2020

Oh, nice suggestions! I will fix it. thanks so much @aromalanil

@aromalanil
Copy link
Owner

aromalanil commented Oct 1, 2020

@fernandoamz I didn't notice this at first but did you really intend to use word Geo Localisation. The apt word would be Geo Location.

And also a state is not needed for an error message. Also, It is not the best practice to return an error message, you can consider throwing an error.

@fernandoamz
Copy link
Contributor Author

Hey!, @aromalanil, I followed up all your suggestions. Let me know if I need to do some other changes!.

@aromalanil aromalanil merged commit 6ef02e0 into aromalanil:master Oct 2, 2020
@aromalanil
Copy link
Owner

LGTM 👍🏻

@aromalanil
Copy link
Owner

@all-contributors please add @fernandoamz for code and doc

@allcontributors
Copy link
Contributor

@aromalanil

I've put up a pull request to add @fernandoamz! 🎉

Copy link
Owner

@aromalanil aromalanil left a comment

Choose a reason for hiding this comment

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

Changes made. approved for merging

useGeolocalisation.js Outdated Show resolved Hide resolved
useGeolocalisation.js Outdated Show resolved Hide resolved
useGeolocalisation.js Outdated Show resolved Hide resolved
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.

None yet

3 participants