Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Content insets #3627

Closed
1ec5 opened this issue Jan 20, 2016 · 11 comments
Closed

Content insets #3627

1ec5 opened this issue Jan 20, 2016 · 11 comments
Assignees
Labels
Android Mapbox Maps SDK for Android

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jan 20, 2016

Content insets were implemented in mbgl and for iOS and OS X in #3583. MapView should expose some sort of inset or padding property so that the map’s center can be shifted downward to account for e.g. a translucent or floating toolbar at the top.

/cc @tobrun

@bleege
Copy link
Contributor

bleege commented Jan 27, 2016

I'm going to review @lgeromegnace's PR in #3687 and then build an example using it in the TestApp so that we can verify it and demonstrate to others how it's used.

@bleege
Copy link
Contributor

bleege commented Jan 27, 2016

The API build out from #3687 has now been been reviewed and merged into master. Will build out example next.

@bleege
Copy link
Contributor

bleege commented Jan 27, 2016

I set the location to be center on Nashville and then took screenshots of with and without a top offset of 250 pixels. Not much has changed and this is likely due to density not being factored in to the API.

device-2016-01-27-135809
No Content Offset

device-2016-01-27-135905
Top Content Offset Of 250px

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 27, 2016

@bleege, this is because setting the insets alone does not move the map. On iOS and OS X, calling -setContentInset: causes the map to shift with optional animation. The Android SDK should similarly handle that detail automatically for client code.

@bleege
Copy link
Contributor

bleege commented Jan 27, 2016

Thanks for the clarification @1ec5. To help illustrate this better (and the fact that we still have API work todo), I added a Marker to the map.

device-2016-01-27-145136
No Content Offset

device-2016-01-27-145226
Top Content Offset Of 1000px

@bleege
Copy link
Contributor

bleege commented Jan 27, 2016

Looking at the Google Maps API the only place (that I can find) that they expose any type of content insets is in CameraUpdateFactory, specifically with two newLatLngBounds() methods that take padding parameters. The padding is uniformly applied in all directions, which is different from Core GL that gives 4, count 'em 4!, directions to specify. I'm thinking that the best bet will be implement these methods that allow for only the single value of padding and then overload them by providing methods that can handle all 4.

I'm also wondering about timing for this as the manual setLatLng(), setZoomLevel(), etc type methods will be removed in 4.0.0 as part of the MapboxMap ticket. This will leave only the Camera API as the way to manipulate the map. It may make more sense to comment out the bindings methods for setContentPadding() in master for now and implement this all for 4.0.0 rather than ship API methods that aren't fully supported for 3.2.0.

/cc @zugaldia @tobrun

screen shot 2016-01-27 at 4 58 11 pm
CameraUpdateFactory.newLatLngBounds() methods

screen shot 2016-01-27 at 5 02 43 pm
Definition of padding

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 27, 2016

I think this is a good place to diverge from what the Google Maps SDK provides. We started with a uniform padding concept in GL JS, but in mapbox/mapbox-gl-js#1675 we decided to allow each side to be specified independently. Uniform padding isn’t suitable for use cases such as a translucent toolbar or floating search bar at the top.

@tobrun
Copy link
Member

tobrun commented Jan 28, 2016

@bleege after reading your thorough analysis, it seems you have a concrete gameplan! I'm now thinking about landing #3145 without LatLngBounds integration. This is one of the API's that is not high on my list to finish before merging. Is this something you want to pick up after?

@bleege
Copy link
Contributor

bleege commented Jan 28, 2016

@tobrun Yep, let's punt on #3627 (Content Insets) until 4.0.0. I'll add a ticket to comment out the setContentPadding() methods to keep them from being used in 3.2.0.

This is one of the API's that is not high on my list to finish before merging. Is this something you want to pick up after?

Nope, the Content Insets don't have to be included in the initial iteration on the MapboxMap work in #3145 . That said this is likely going to be an important feature for future Navigation work so working on this immediately after MapboxMap lands will be important. It sounds like you have things well underway with this from your work in #3145 so I'll let you finish #3627 off at that time.

@bleege bleege assigned tobrun and unassigned bleege Jan 28, 2016
@tobrun
Copy link
Member

tobrun commented Jan 28, 2016

@bleege 👍

@tobrun
Copy link
Member

tobrun commented Feb 10, 2016

As mentioned in #3761 (comment) it seems that Google Maps SDK is exposing this as GoogleMap.setPadding(int,int,int,int). I'm going to implement this in the same fashion. Will close this in favour of the feature request in #3761.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

No branches or pull requests

4 participants