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

Fit to supplied markers #386

Merged
merged 4 commits into from
Jul 22, 2016

Conversation

AidenMontgomery
Copy link
Contributor

This is a cleaned up version of an earlier pull request, which is based on a more recent fork of the master branch.

@@ -53,6 +53,7 @@
| `animateToRegion` | `region: Region`, `duration: Number` |
| `animateToCoordinate` | `region: Coordinate`, `duration: Number` |
| `fitToElements` | `animated: Boolean` |
| `fitToSpecifiedMarkers` | `markerIDs: String[]` |

Copy link
Collaborator

@christopherdro christopherdro Jul 12, 2016

Choose a reason for hiding this comment

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

Should be fitToSuppliedMarkers

@christopherdro
Copy link
Collaborator

@AidenMontgomery Thanks for redoing the PR!

Looks good to me but I haven't gotten a chance to test it.

/cc @jrichardlai @lelandrichardson @felipecsl

I think we should get this merged in.

if (feature instanceof AirMapMarker) {
String identifier = ((AirMapMarker)feature).getIdentifier();
Marker marker = (Marker)feature.getFeature();
if (Arrays.asList(markerIDs).contains(identifier)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

extract Arrays.asList(markerIDs) as a variable outside of the loop so you dont allocate a new array every time

@felipecsl
Copy link
Contributor

looks good to me with minor nits 👍

@AidenMontgomery
Copy link
Contributor Author

Thanks for the feedback. I will make the changes and do some more testing this evening, then update the PR.

@AidenMontgomery
Copy link
Contributor Author

AidenMontgomery commented Jul 13, 2016

I haven't tested this on Android yet, but I have run the example project on the iOS Simulator and I think that it looks ok. I am going to try and get it running on the Android emulator now.

@christopherdro
Copy link
Collaborator

@AidenMontgomery I should have some time today to test these on a few android devices.

@christopherdro
Copy link
Collaborator

Remind me again why you wouldn't use fitToElelments and instead of having to specify an array of marker ids?

@AidenMontgomery
Copy link
Contributor Author

Unless it has changed recently, fitToElements shows every marker on the map. In my use case I needed to zoom to the closest 2 markers on the map, relative to another marker. However, as soon as the user zooms out they need to be able to see all of the markers in the area. I could have done this by removing all other markers, then reacting to the region changed gesture and adding all of the other markers back on the map, but this felt simpler to me.

@erickreutz
Copy link

Would love to be able to use this!

@christopherdro
Copy link
Collaborator

@felipecsl I think your comments were addressed. Should we get this merged in?

@felipecsl
Copy link
Contributor

yep, looks good to me!

@christopherdro christopherdro merged commit 980628c into react-native-maps:master Jul 22, 2016
BigPun86 pushed a commit to BigPun86/react-native-maps that referenced this pull request Dec 1, 2016
* Added support for zooming the map to specific markers

* Moved list creation outside of a loop in Android. Updated documentation for fitToSuppliedMarkers

* Tidied up the fitToSuppliedMarkers example

* Updated Android gif in the readme
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.

4 participants