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

adding onMyLocationChange event #2032

Merged
merged 7 commits into from Mar 4, 2018

Conversation

Projects
None yet
8 participants
@dabit1
Copy link
Contributor

dabit1 commented Feb 18, 2018

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

@rborn
Copy link
Collaborator

rborn left a comment

@alvelig LGTM 🐽

@alvelig

This comment has been minimized.

Copy link
Collaborator

alvelig commented Feb 19, 2018

@dabit1 Thank you so much for such an important contribution!
May I ask you to make a simple example on how to use it (or how you use it)?

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Feb 20, 2018

@dabit1 ping for docs/example 🤗

@dabit1

This comment has been minimized.

Copy link
Contributor Author

dabit1 commented Feb 20, 2018

@bramus

This comment has been minimized.

Copy link
Contributor

bramus commented Feb 20, 2018

Oh, this one would be awesome! Nice work!

@dabit1

This comment has been minimized.

Copy link
Contributor Author

dabit1 commented Feb 24, 2018

@alvelig there is an example file changed with this new feature on this PR. Is it ok?

@Bosseskompis

This comment has been minimized.

Copy link

Bosseskompis commented Mar 3, 2018

Any update on this?

@alvelig

This comment has been minimized.

Copy link
Collaborator

alvelig commented Mar 3, 2018

@dabit1 LGTM sorry for the delay, please resolve conflicts and we merge. Actually it's about adding from both branches.

@alvelig

This comment has been minimized.

Copy link
Collaborator

alvelig commented Mar 3, 2018

And also do you ming renaming onMyLocationChange to onUserLocationChange as it will be more consistent with showsUserLocation

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 3, 2018

@dabit1

This comment has been minimized.

Copy link
Contributor Author

dabit1 commented Mar 3, 2018

dabit1 and others added some commits Mar 4, 2018

@dabit1

This comment has been minimized.

Copy link
Contributor Author

dabit1 commented Mar 4, 2018

@alvelig done!

David

@alvelig alvelig merged commit 82c953d into react-native-community:master Mar 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zerazeru

This comment has been minimized.

Copy link

zerazeru commented Mar 5, 2018

There's a } missing in AIRGoogleMap.m. PR's get merged without even trying to build before..? :/

Anyway, after fixing it, the event shows up, but it comes without all the location data (on ios).

@bramus

This comment has been minimized.

Copy link
Contributor

bramus commented Mar 5, 2018

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 coordinate.putFloat() calls because it would not compile otherwise. Once I got it running it's not not working:

  1. when playing back a .gpx file via the Emulator
  2. when manually sending a new location to the Emulator

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.

alvelig added a commit that referenced this pull request Mar 5, 2018

@bramus

This comment has been minimized.

Copy link
Contributor

bramus commented Mar 6, 2018

With the hotfix above applied I've done some more testing in the Simulators:

  • iOS: builds + works
  • Android: builds + doesn't work (neither when playing back a .gpx file via the Emulator nor when sending over new location to the Emulator)
@zerazeru

This comment has been minimized.

Copy link

zerazeru commented Mar 6, 2018

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.
Could it be related to RCTBubblingEventBlock / RCTDirectEventBlock ?

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 6, 2018

@bramus on android same issues with you no location is triggered, not even on android side - I've put a Log.v in the event (tested on a genymotion). Waiting for my android device to charge and I'll test for real too.

@alvelig

This comment has been minimized.

Copy link
Collaborator

alvelig commented Mar 6, 2018

I've tested in the Emulator with example. Does it work for you, guys?

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 6, 2018

@alvelig as I said, no

@bramus

This comment has been minimized.

Copy link
Contributor

bramus commented Mar 6, 2018

Same here @rborn (added some log statements in the handling Java code … no result).

On iOS I see the events come in @zerazeru (Tested with Debug > Location > Bicycle Ride):

screen shot 2018-03-06 at 13 15 46

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 6, 2018

@bramus @alvelig Ok, figured out, the example app doesn't ask for permissions on start 😳
You need to go and enable location manually in the Settings then it will work

@bramus

This comment has been minimized.

Copy link
Contributor

bramus commented Mar 6, 2018

Weird, as the AndroidManifest.xml file does require 'm:

    <uses-permission android:name="android.permission.ACCESS_COURSE_LOCATION"/>
    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />

🤔

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 6, 2018

@bramus

This comment has been minimized.

Copy link
Contributor

bramus commented Mar 6, 2018

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 :))

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 6, 2018

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

🤔

@alvelig

This comment has been minimized.

Copy link
Collaborator

alvelig commented Mar 6, 2018

Consider #1489

@bramus

This comment has been minimized.

Copy link
Contributor

bramus commented Mar 6, 2018

I wouldn't include react-native-permissions in the core itself, as it would only broaden the surface react-native-maps is covering. Including in the examples (to make sure they work) and mentioning it in the documentation (to not confuse those checking out the examples, like I was now) should do the job.

For an app I built I created this “Permissions Required” component which uses react-native-permissions under the hood. The text is in Dutch, but you get the idea: it asks for permission to use your location, and only after having granted the required permissions the <MapView /> is being rendered.

screenshot

With some adjustments this could make it into the examples.

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 7, 2018

@bramus I totally agree with adding some docs about this

@aMarCruz

This comment has been minimized.

Copy link
Contributor

aMarCruz commented Apr 3, 2018

@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;
@dabit1

This comment has been minimized.

Copy link
Contributor Author

dabit1 commented Apr 3, 2018

@aMarCruz feel free to make a PR ;)

@aMarCruz

This comment has been minimized.

Copy link
Contributor

aMarCruz commented Apr 4, 2018

@dabit1 , done :) PR is #2165

@dabit1

This comment has been minimized.

Copy link
Contributor Author

dabit1 commented Apr 5, 2018

@aMarCruz well done!

@AakashKB

This comment has been minimized.

Copy link

AakashKB commented May 21, 2018

For some reason, when I use onUserLocationChange, I get an undefined latitude and longitude.

My code:

<MapView
          ref={map => (this._mapView = map)}
          style={styles.map}
          showsUserLocation={true}
          showsPointsOfInterest={false}
          followsUserLocation={false}
          provider="google"
          region={this.state.region}
          onRegionChange={region => this.state.region = region}
          onUserLocationChange={location => alert(location.latitude)}
          onLongPress={event =>
            this.createMapEvent(event.nativeEvent.coordinate)
          }
        >
@dabit1

This comment has been minimized.

Copy link
Contributor Author

dabit1 commented May 22, 2018

@AakashKB are permissions enabled?

@AakashKB

This comment has been minimized.

Copy link

AakashKB commented May 23, 2018

@dabit1 yes, location permissions are enabled. showUserLocation works well.

@AakashKB

This comment has been minimized.

Copy link

AakashKB commented May 23, 2018

onuserlocationchange

I get this when I print out Object.keys(location). There is no key for latitude or longitude even returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.