-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
adding onMyLocationChange event #2032
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alvelig LGTM 🐽
@dabit1 Thank you so much for such an important contribution! |
@dabit1 ping for docs/example 🤗 |
😂 I'm so busy sorry. Maximum Sunday I hope do that.
El 20 feb. 2018 7:13 p. m., "Dan Tamas" <notifications@github.com> escribió:
… @dabit1 <https://github.com/dabit1> ping for docs/example 🤗
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2032 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AWN9bo5LNAQ6jkVBKl97beOIMS7nt0a4ks5tWwtHgaJpZM4SJq9S>
.
|
Oh, this one would be awesome! Nice work! |
@alvelig there is an example file changed with this new feature on this PR. Is it ok? |
Any update on this? |
@dabit1 LGTM sorry for the delay, please resolve conflicts and we merge. Actually it's about adding from both branches. |
And also do you ming renaming |
+1 for renaming 🤗
…Sent from my iPhone
On 3 Mar 2018, at 20:49, Alexander Veligurov ***@***.***> wrote:
And also do you ming renaming onMyLocationChange to onUserLocationChange as it will be more consistent with showsUserLocation
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Ey guys tomorrow afternoon I will do it. 😎
El 3 mar. 2018 11:07 p. m., "Dan Tamas" <notifications@github.com> escribió:
… +1 for renaming 🤗
Sent from my iPhone
> On 3 Mar 2018, at 20:49, Alexander Veligurov ***@***.***>
wrote:
>
> And also do you ming renaming onMyLocationChange to onUserLocationChange
as it will be more consistent with showsUserLocation
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2032 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AWN9bhCG8B-ZdAafbR-gm2aBGNNih_2iks5taxQUgaJpZM4SJq9S>
.
|
@alvelig done! |
There's a Anyway, after fixing it, the event shows up, but it comes without all the location data (on ios). |
Just tested out this PR using the example provided (up to the point before the rename) in the Android Emulator. I had to disable the
In both cases I don't see any location change events :( Is it possible to revert this PR merge, awaiting a proper/better implementation? It'd also unbreak the master (see #2058) along with that. |
With the hotfix above applied I've done some more testing in the Simulators:
|
On iOS it builds for me but there is no event data (no coordinates), only a Proxy object. Does it work for you? I noticed that it's the same for onPress, but for example for onRegionChange the proper event object is received. |
@bramus on android same issues with you no location is triggered, not even on android side - I've put a |
I've tested in the Emulator with example. Does it work for you, guys? |
@alvelig as I said, no |
Weird, as the
🤔 |
Aha, that could be it indeed … should we update the code to include something like https://github.com/yonahforst/react-native-permissions to request for location permissions first, in order to prevent confusion like we're having now? (I'm using this library in an already deployed app, works fine :)) |
I like that library too but I'm not sure if we should suggest using it in the docs or try to do it inside this module so we don't force the user to use an extra module. On the other hand that library gives way more flexibility and would allow a better UX 🤔 |
Consider #1489 |
I wouldn't include For an app I built I created this “Permissions Required” component which uses With some adjustments this could make it into the examples. |
@bramus I totally agree with adding some docs about this |
@dabit1 please add typings for all of us using Typescript :) onUserLocationChange?: (value: {
nativeEvent: { coordinate: {
latitude: number,
longitude: number,
altitude: number,
accuracy: number,
speed: number,
isFromMockProvider: boolean
}}
}) => void; |
@aMarCruz feel free to make a PR ;) |
@aMarCruz well done! |
For some reason, when I use onUserLocationChange, I get an undefined latitude and longitude. My code:
|
@AakashKB are permissions enabled? |
@dabit1 yes, location permissions are enabled. showUserLocation works well. |
A fork from #1508 with no conflicts. Thanks so much @sagi . He is the author of the pull request and the thanks must go to him but It has conflicts since July, 2017. I think it is a great PR and must be merged.
I also added more data to event: altitude, accuracy, altitude_accuracy and speed.
@alvelig Please could you merge this awesome PR?
Regards,
David Escalera