Feature/fit to coordinates #545

naoufal · 2016-09-06T23:52:06Z

Overview

Adds a fitToCoordinates method to <MapView />
Adds documentation for the method
Adds an example

Demo

Why?

I'm implementing a UI that requires the map to focus on a polyline or mix of polyline and markers. The current fitToSuppliedMarkers method comes short as it only supports markers. At first, I thought about adding and identifier prop to polylines, but I decided against it since one might want to focus on a specific part of a polyline. I settled on coordinates as I feel it's a more flexible API that covers a wider range of use cases.

Questions

I can see some people omitting edgePadding which would have function calls look like fitToCoordinates(coords, null, true). It's not the prettiest, so I was hoping to get feedback on adding a second method for developers that require edgePadding. Something like:

class MapView extends React.Component {
  // ...

  fitToCoordinates(coordinates, animated) {
    this.fitToCoordinates(coordinates, undefined, animated);
  }

  fitToCoordinatesWithEdgePadding(coordinates, edgePadding, animated) {
    this._runCommand('fitToCoordinates', [coordinates, edgePadding, animated]);
  }

  // ...
}

To do

Validate API - @naoufal
Add Android support - @chrisknepper
Update readme with example - @naoufal

lelandrichardson · 2016-09-07T23:02:52Z

@naoufal wow, this looks really good! Is there any way you'd be willing to tackle adding this API to android as well? We are trying our best to keep this component have feature parity cross-platform.

naoufal · 2016-09-07T23:05:40Z

@lelandrichardson Yeah, I can give it a shot this weekend.

What are you thoughts on fitToCoordinates vs fitToCoordinatesWithEdgePadding?

spikebrehm · 2016-09-07T23:13:22Z

components/MapView.js

@@ -399,6 +399,10 @@ class MapView extends React.Component {
    this._runCommand('fitToSuppliedMarkers', [markers, animated]);
  }

+  fitToCoordinates(coordinates, edgePadding, animated) {


What do you think about providing default arguments? Something like:

fitToCoordinates(coordinates, edgePadding = {}, animated = true) {

@spikebrehm I initially had { top: 0, right: 0, bottom: 0, left: 0 } as a default argument, but then realized setVisibleMapRect could be called without edgePadding altogether. So I moved the conditional to the native side.

I don't mind switching to the default argument instead, whatever you guys prefer -- I'm easy.

Maybe we could also add some documentation to this method that explains the possible keys of edgePadding. Either with a comment, or again using default arguments as documentation:

fitToCoordinates(coordinates, edgePadding = { top = 0, right = 0, bottom = 0, left = 0 }, animated = true) {

@spikebrehm I'll update it to use the default arguments.

As for additional documentation, I included an EdgePadding type to the MapView docs

spikebrehm · 2016-09-07T23:15:08Z

Looks really nice! I'm partial to just a single fitToCoordinates() with default arguments, but I could be convinced either way.

naoufal · 2016-09-07T23:20:21Z

@spikebrehm I'm also for a single fitToCoordinates.

lelandrichardson · 2016-09-07T23:21:41Z

fitToCoordinates is fine, as long as we preserve the option for providing a padding amount

chrisknepper · 2016-09-09T23:02:53Z

@naoufal Let me know if you'd like any assistance with the Android stuff!

This would be a great feature for sure. I implemented similar behavior for my project in JS but if we can make it a native feature of the plugin, all the better.

naoufal · 2016-09-10T20:20:10Z

@chrisknepper That'd be great! I'm struggling to get my Android environment setup. You can get in touch with me on the Reactiflux chat if you have any questions.

chrisknepper · 2016-09-12T22:51:55Z

@naoufal I think I have gotten this working on Android (except for edgePadding). I'll be contacting you shortly over at Reactiflux.

naoufal · 2016-09-12T22:53:08Z

@chrisknepper Looks great!

spikebrehm · 2016-09-13T16:04:20Z

android/src/main/java/com/airbnb/android/react/maps/AirMapView.java

@@ -549,6 +549,33 @@ public void fitToSuppliedMarkers(ReadableArray markerIDsArray, boolean animated)
        }
    }

+    public void fitToCoordinates(ReadableArray coordinatesArray, ReadableMap edgePadding, boolean animated) {
+        //Log.d("AirMapView", "running thru the 6 with my woes");


Whoops, forgot to remove that last night 😪

naoufal · 2016-09-13T16:29:00Z

@chrisknepper Is the Android part ready for review?

felipecsl · 2016-09-13T17:07:43Z

android/src/main/java/com/airbnb/android/react/maps/AirMapView.java

+        LatLngBounds bounds = builder.build();
+        CameraUpdate cu = CameraUpdateFactory.newLatLngBounds(bounds, 50);
+
+        if(edgePadding != null) {


formatting nit

@chrisknepper You're missing a space after your if on line 566.

chrisknepper · 2016-09-13T19:57:08Z

@naoufal It is now ready for review. I'm not sure where the conflicts are but we should merge in the latest from here into your fork.

naoufal · 2016-09-17T18:16:47Z

@spikebrehm, @lelandrichardson Alright, so I updated my changes based on the feedback provided, added an example to the main README and rebased off master to resolve the conflicts. @chrisknepper and I have both tested the example on our respective Android and iOS environments.

Can you run the examples on your machines and let me know if there's anything else I can do to help move this PR forward.

chrisknepper · 2016-09-19T14:36:35Z

I have one suggestion for this. IMO, edgePadding should come after animated in the parameters. For the case when a user doesn't want to use edgePadding but wants animated to be true, they will have to explicitly pass null as the argument for edgePadding

This results in the call looking like this: fitToCoordinates(coords, null, true).
If animated was the 2nd parameter, it could be fitToCoordinates(coords, true).

This isn't a big deal and is just a semantics preference, but I envision this being frequently used without edgePadding. Just my two cents.

naoufal · 2016-09-19T15:23:54Z

I'm in favour of keeping animated as the last argument for consistency. In my experience, edgePadding is used more frequently than animated for this use case. Not passing an edgePadding cuts the surrounding markers, so you typically need to add edgePadding to get the result you're looking for. You can see this first hand in the [example](https://github.com/airbnb/react-native-maps/pull/545/files#diff-218c7392a17180cda7a16429d00018aeR35). So I feel like passingnull` might be a rarer use case than you might think. I'd also prefer to have animated be the last argument in our methods, similarly to Cocoa methods.

I'd like to hear @spikebrehm and @lelandrichardson's thoughts on this.

lelandrichardson · 2016-09-20T17:44:42Z

Boolean parameters make me sad. I wonder if a better approach is fitToCoordinates(coordinates, options).

option could specify animated: false (animated: true would be default).
option could also have edgePadding: { ... } which would default to null, or 0, 0.

Just an idea. What do you all think?

spikebrehm · 2016-09-20T18:24:45Z

options is a good idea for optional parameters. That's my vote as well, but I don't feel strongly either way.

Also: note there's a merge conflict in example/App.js.

naoufal · 2016-09-20T19:59:16Z

@lelandrichardson an options argument would solve this. My only concern is what it means for consistency since none of the other APIs accept options. We can probably go back and forth with this though, so I'll let you guys decide which interface you prefer and I'll gladly adjust the PR.

@spikebrehm Conflicts keep popping up as other PRs get merged in. I'm going to hold off fixing them until we've decided on the final API to avoid duplicating work.

athaeryn · 2016-09-20T20:28:30Z

In #495 I was working on adding props to the map component to set pixel insets. The work there isn't complete, but I'm wondering if maybe having those inset props and a fitToCoordinates function that doesn't need to accept an edge padding argument might be a better design.

spikebrehm · 2016-09-20T21:04:32Z

We can probably go back and forth with this though, so I'll let you guys decide which interface you prefer and I'll gladly adjust the PR.

@naoufal sounds good! @lelandrichardson and I can migrate the other methods to take options before we cut the next breaking change.

spikebrehm · 2016-09-22T17:09:18Z

@athaeryn

In #495 I was working on adding props to the map component to set pixel insets. The work there isn't complete, but I'm wondering if maybe having those inset props and a fitToCoordinates function that doesn't need to accept an edge padding argument might be a better design.

Good point! If this gets merged before #495, we can cut a new release that utilizes pixel insets.

@naoufal let's go with the options arg, rebase this from master, and get this merged!

naoufal · 2016-09-22T17:17:14Z

@spikebrehm Sounds good to me. I'll make the changes tonight and ping you 👍

-Remove extraenous comment

naoufal · 2016-09-23T00:02:17Z

@spikebrehm Added the options argument and rebased. I'm good to go on my end.

spikebrehm · 2016-09-23T00:22:08Z

Beautiful, thank you! 🍻

leandrobortolotto · 2016-12-16T19:09:48Z

@naoufal I'm trying to use fitToCoordinates but without success. I'm on Mac OS and building to an iOS simulator. I can add markers but this method doesn't work. Here it's part of my code:

`const MARKER_COORDINATES = [ {latitude: -27.570351,longitude: -48.513944},
{latitude: -27.574329,longitude: -48.526673} ];

...

const DEFAULT_PADDING = { top: 40, right: 40, bottom: 40, left: 40 };

export default class TesteMapa extends Component {

fitAllMarkers() {
console.log()
console.log( this.map );
console.log( MARKER_COORDINATES.map );
this.map.fitToCoordinates(MARKER_COORDINATES, {
edgePadding: DEFAULT_PADDING,
animated: true,
});
console.log( "Method finished" );
}

`

I can see the log messages, etc. What am I missing?

Thanks

avet-m · 2018-12-17T09:55:38Z

Zoom level is too bug, when passing only one coordinate

_fitToCoordinates = (destSnapPoint) => {
        this.mapRef.current._component.fitToCoordinates(
            [{
                latitude: 59.978315,
                longitude: 30.314519,
                latitudeDelta: 0.02,
                longitudeDelta: 0.02,
            }],
            {
                edgePadding: {
                    top: 0,
                    right: 0,
                    bottom: destSnapPoint,
                    left: 0,
                },
                animated: true,
            },
        );
    };

#2629

diegorodriguesvieira · 2020-02-27T13:08:40Z

@naoufal Can you help me with fitToCoordinates, because it are ignoring mapPadding

#3308

spikebrehm reviewed Sep 7, 2016
View reviewed changes

spikebrehm reviewed Sep 13, 2016
View reviewed changes

felipecsl reviewed Sep 13, 2016
View reviewed changes

naoufal force-pushed the feature/fit-to-coordinates branch from ab63b62 to 7742e6c Compare September 17, 2016 18:09

naoufal and others added 4 commits September 22, 2016 19:43

Add fitToCoordinates method

06f152a

Update docs

4b257e0

Add example

a199103

-Port fitToCoordinates functionality over to Android

bcfad6b

chrisknepper and others added 4 commits September 22, 2016 19:45

-Set constant for baseMapPadding used in newLatLngBounds calls

d63e849

-Remove extraenous comment

Add default arguments to fitToCoordinates

3647602

Add edgePadding example

c8986d5

Update README with example

f3dd70c

naoufal force-pushed the feature/fit-to-coordinates branch from df63ec5 to be02f25 Compare September 22, 2016 23:47

Add an options argument

1e81b06

naoufal force-pushed the feature/fit-to-coordinates branch from be02f25 to 1e81b06 Compare September 22, 2016 23:51

spikebrehm merged commit 7a5083c into react-native-maps:master Sep 23, 2016

Feature/fit to coordinates #545

Feature/fit to coordinates #545

Conversation

naoufal commented Sep 6, 2016 • edited Loading

Overview

Demo

Why?

Questions

To do

lelandrichardson commented Sep 7, 2016

naoufal commented Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spikebrehm commented Sep 7, 2016

naoufal commented Sep 7, 2016

lelandrichardson commented Sep 7, 2016

chrisknepper commented Sep 9, 2016

naoufal commented Sep 10, 2016

chrisknepper commented Sep 12, 2016

naoufal commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naoufal commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisknepper commented Sep 13, 2016 • edited Loading

naoufal commented Sep 17, 2016 • edited Loading

chrisknepper commented Sep 19, 2016

naoufal commented Sep 19, 2016

lelandrichardson commented Sep 20, 2016

spikebrehm commented Sep 20, 2016

naoufal commented Sep 20, 2016

athaeryn commented Sep 20, 2016

spikebrehm commented Sep 20, 2016

spikebrehm commented Sep 22, 2016

naoufal commented Sep 22, 2016

naoufal commented Sep 23, 2016

spikebrehm commented Sep 23, 2016

leandrobortolotto commented Dec 16, 2016

avet-m commented Dec 17, 2018

diegorodriguesvieira commented Feb 27, 2020

naoufal commented Sep 6, 2016 •

edited

Loading

naoufal commented Sep 7, 2016 •

edited

Loading

chrisknepper commented Sep 13, 2016 •

edited

Loading

naoufal commented Sep 17, 2016 •

edited

Loading