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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Android support for onPoiClick #2050

Merged
merged 6 commits into from Mar 10, 2018

Conversation

Projects
None yet
3 participants
@julienR2
Copy link
Contributor

julienR2 commented Mar 2, 2018

Exposes the onPoiClick event to react-native-maps wrapper.
It send back the location of the poi, the name and the googleId (docs updated)

I haven't dug in IOS implementation yet since I'm more confortable with android. But I'll definitely do it through a next PR ! (or if anyone is keen to, feel free)

Let me know if any other steps need to be done 馃憤

julienr2
@alvelig

This comment has been minimized.

Copy link
Collaborator

alvelig commented Mar 2, 2018

Great! Can you please also add an example of usage. And we'll have to rebase a little bit later it as there's another PR adding feature to the same files. So if you are going to add iOS implementation, absolutely welcome (will it be only GM specific?)

@julienR2

This comment has been minimized.

Copy link
Contributor Author

julienR2 commented Mar 5, 2018

A pleasure to collaborate !
I'll definitely add an example of usage today 馃憤
And I'll take advantage of the time before the other PR is added, to dig into the IOS implementation. I'm quite new in the SDK-to-React Native behavior so I'll see what I can do ! Ideally I would make it non GM specific but I'll check if it's possible depending on the other maps lib.

@rborn rborn added the waiting reply label Mar 5, 2018

julienr2 added some commits Mar 5, 2018

julienr2
@julienR2

This comment has been minimized.

Copy link
Contributor Author

julienR2 commented Mar 5, 2018

Here is the example ! Let me know if it's good.

There is one thing I still feel weird about, it's to have to get the nativeEvent of the event to get the object define in Android instead of accessing it directly.. (https://github.com/nowmad-io/react-native-maps/blob/ff8e45bd9c2645a9baf6611da875ebc33c672543/example/examples/OnPoiClick.js#L37)

I'm open to update it if there is a better way, that I didn't get yet.

julienr2
@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 6, 2018

LGTM @alvelig 馃惤

julienr2
@julienR2

This comment has been minimized.

Copy link
Contributor Author

julienR2 commented Mar 7, 2018

@rborn @alvelig
I added the support for the onPoiClick event in IOS.
It seems MapKit doesn't expose any event on their POI, so this event is GM specific (I updated the README to reflect the information).

Hope it's all good, let me know otherwise !

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 7, 2018

@julienR2 LGTM, one question only: what's happening if we use the apple map?
Are there any drawbacks?

@julienR2

This comment has been minimized.

Copy link
Contributor Author

julienR2 commented Mar 8, 2018

@rborn Since the callback doesn't exist on MapKit, I implemented it only on AirGoogleMaps. Therefore if you try using onPoiClick callback on with native IOS map, nothing will happen since no listener nor callback are set.

I'm wondering if it would be worth logging a warning AirMap doesn't provide onPoiClick event in case we try to set the callback on an IOS env, but I don't recall seeing this kind of practice in the rest of the lib.

Let me know if it's needed !

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 8, 2018

Thats great, and you specify in the docs it's google only so no need for the log.

@@ -50,6 +50,7 @@ To access event data, you will need to use `e.nativeEvent`. For example, `onPres
| `onRegionChangeComplete` | `Region` | Callback that is called once when the region changes, such as when the user is done moving the map.
| `onPress` | `{ coordinate: LatLng, position: Point }` | Callback that is called when user taps on the map.
| `onPanDrag` | `{ coordinate: LatLng, position: Point }` | Callback that is called when user presses and drags the map. **NOTE**: for iOS `scrollEnabled` should be set to false to trigger the event
| `onPoiClick` | `{ coordinate: LatLng, position: Point, placeId: string, name: string }` | Callback that is called when user click on a POI. **NOTE**: Android only (IOS soon)

This comment has been minimized.

Copy link
@rborn

rborn Mar 8, 2018

Collaborator

We have iOS now, right? So the soon line can be removed

poi: null,
};

this.onPoiClick = this.onPoiClick.bind(this);

This comment has been minimized.

Copy link
@rborn

rborn Mar 8, 2018

Collaborator

If you use an arrow function the bind is not needed, but it's up to you :)

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 8, 2018

@julienR2 small comments, then we merge 馃帀

@julienR2

This comment has been minimized.

Copy link
Contributor Author

julienR2 commented Mar 9, 2018

Oups well seen 馃憤 I forgot to change this after implementing IOS. I'll do it immediately !

@rborn rborn merged commit 624ba2a into react-native-community:master Mar 10, 2018

1 check passed

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

@julienR2 julienR2 deleted the nowmad-io:on-poi-click branch Mar 10, 2018

@julienR2

This comment has been minimized.

Copy link
Contributor Author

julienR2 commented Mar 10, 2018

Awesome ! Thanks for merging it 馃憤

@julienR2 julienR2 restored the nowmad-io:on-poi-click branch Mar 13, 2018

@julienR2 julienR2 deleted the nowmad-io:on-poi-click branch Mar 23, 2018

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