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

Fix for pointForCoordinate and coordinateForPoint #2039

Merged

Conversation

Projects
None yet
3 participants
@wasedaigo
Copy link
Contributor

wasedaigo commented Feb 22, 2018

  • Fixed typo "coordinateFromPoint"
  • Moved coordinateForPoint/pointForCoordinate from AirMap to AirMapManager
  • Fixed how resolve/reject are handled

I have tested on iOS. Yet not confirmed on Android. If anybody can confirm this works on Android, I think this PR is good to go.

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Feb 22, 2018

LGTM @alvelig 🐽
PS - not tested on android yet

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Feb 22, 2018

@wasedaigo does it work on iOs for both Apple maps and Google maps ?
also how's compared to this: #1737

🤗

@rborn rborn added the waiting reply label Feb 22, 2018

@wasedaigo

This comment has been minimized.

Copy link
Contributor Author

wasedaigo commented Feb 22, 2018

@rborn I haven't tested GoogleMap, but it seems it won't work. Sounds like we can integrate part of #1737 to make it work. What would be the best approach here? I can add the implementation, but I have no time to test. It is hard to test all combinations. As these methods do not work on GoogleMap/Android anyways, as long as it compiles we can merge this and move forward?

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Feb 22, 2018

@wasedaigo @alvelig I will try to test on android tomorrow and also if we can merge first #1737 and then this one.
If problems arise we might wanna integrate that PR in yours. OR if you know for sure that will be problems you could try to integrate it and update the PR. And I'll test.

Works for you?

@wasedaigo

This comment has been minimized.

Copy link
Contributor Author

wasedaigo commented Feb 22, 2018

actually #1737 should be closed, the implementation for GoogleMap is already there.
It is just this commit 9cb22b9 made a mistake on iOS AppleMap part

I think once you confirmed this works on Android, we are good.

@wasedaigo

This comment has been minimized.

Copy link
Contributor Author

wasedaigo commented Feb 26, 2018

@rborn is everything alright? I want to make sure I am not blocking you

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Feb 26, 2018

@wasedaigo sorry, didn't have the time to look into and the next days I'll be very busy. I'll try to take a look in the next evenings but I don't promise 😔

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 8, 2018

@wasedaigo it works great on android ❤️

Sorry for the delay.

@rborn rborn merged commit 5f7bb26 into react-native-community:master Mar 8, 2018

1 check passed

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

This comment has been minimized.

Copy link

henninghall commented Mar 13, 2018

I found some issues related to this pull request.

  1. coordinateForPoint with Google maps does not work on iOS
  2. coordinateForPoint resolves coordinate on format {lat, lng} on iOS with apple maps and {latitude,longitude} with google maps on android.
@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Mar 13, 2018

@wasedaigo can you have a look at this ?

danielgindi added a commit to danielgindi/react-native-maps that referenced this pull request Apr 1, 2018

rborn added a commit that referenced this pull request May 1, 2018

@mbrehme mbrehme referenced this pull request Sep 30, 2018

Closed

Update MapView.js #2522

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.