Skip to content

Fit to supplied markers#386

Merged
christopherdro merged 4 commits intoreact-native-maps:masterfrom
ConstructiveCoding:master
Jul 22, 2016
Merged

Fit to supplied markers#386
christopherdro merged 4 commits intoreact-native-maps:masterfrom
ConstructiveCoding:master

Conversation

@AidenMontgomery
Copy link
Copy Markdown
Contributor

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

| `animateToCoordinate` | `region: Coordinate`, `duration: Number` |
| `fitToElements` | `animated: Boolean` |
| `fitToSpecifiedMarkers` | `markerIDs: String[]` |

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

looks good to me with minor nits 👍

@AidenMontgomery
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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

@christopherdro
Copy link
Copy Markdown
Collaborator

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

@AidenMontgomery
Copy link
Copy Markdown
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
Copy Markdown

Would love to be able to use this!

@christopherdro
Copy link
Copy Markdown
Collaborator

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

@felipecsl
Copy link
Copy Markdown
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
pinpong pushed a commit to pinpong/react-native-maps that referenced this pull request Feb 28, 2025
* 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