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

julienR2
Copy link
Contributor

@julienR2 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 馃憤

@alvelig
Copy link
Contributor

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
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.

@julienR2
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.

@rborn
Copy link
Collaborator

rborn commented Mar 6, 2018

LGTM @alvelig 馃惤

@julienR2
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
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
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
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.

docs/mapview.md Outdated
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

poi: null,
};

this.onPoiClick = this.onPoiClick.bind(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@rborn
Copy link
Collaborator

rborn commented Mar 8, 2018

@julienR2 small comments, then we merge 馃帀

@julienR2
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-maps:master Mar 10, 2018
@julienR2 julienR2 deleted the on-poi-click branch March 10, 2018 13:50
@julienR2
Copy link
Contributor Author

Awesome ! Thanks for merging it 馃憤

@julienR2 julienR2 restored the on-poi-click branch March 13, 2018 04:21
@julienR2 julienR2 deleted the on-poi-click branch March 23, 2018 09:43
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