Allow user to specify n, s, e, w padding options for fitBounds #3890

mollymerp · 2017-01-04T18:21:18Z

Launch Checklist

Continuing on @tristen's PR from #1682.
In order to have separate padding pixels on each side of the map bounds. Right now its set up as [n,e,s,w] but I'm not sure this makes the most sense. Should we be consistent with the offset option and take 2 Points (one for each corner of the bounds)?

~~Testing the debug page is showing me I'm slightly off on the adjustments right now so I have to figure out what I did wrong in adjusting the camera function.~~ flipped x, y 😭

cc @lucaswoj
closes #1339

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page

mollymerp · 2017-01-05T21:14:38Z

ready for your 👀 @lucaswoj

lucaswoj · 2017-01-09T22:25:14Z

js/ui/camera.js

@@ -303,32 +303,58 @@ class Camera extends Evented {
     *     {@link Map#easeTo}. If `false`, the map transitions using {@link Map#flyTo}. See
     *     {@link Map#flyTo} for information about the options specific to that animated transition.
     * @param {Function} [options.easing] An easing function for the animated transition.
-     * @param {number} [options.padding=0] The amount of padding, in pixels, to allow around the specified bounds.
+     * @param {number|Object} [options.padding] Sets padding around the given bounds on each side in pixels. Accepts a number for all padding edges or an object with padding for each compass direction (north, south, east, and west)


We can provide a more specific type definition by creating a PaddingOptions @typdef for {north: Number; east: Number; south: Number; west: Number;}.

ok! will investigate

lucaswoj · 2017-01-09T22:26:51Z

js/ui/camera.js

@@ -303,32 +303,58 @@ class Camera extends Evented {
     *     {@link Map#easeTo}. If `false`, the map transitions using {@link Map#flyTo}. See
     *     {@link Map#flyTo} for information about the options specific to that animated transition.
     * @param {Function} [options.easing] An easing function for the animated transition.
-     * @param {number} [options.padding=0] The amount of padding, in pixels, to allow around the specified bounds.
+     * @param {number|Object} [options.padding] Sets padding around the given bounds on each side in pixels. Accepts a number for all padding edges or an object with padding for each compass direction (north, south, east, and west)


What are your thoughts about providing geographic padding options (north, east, ...) vs screen padding options (top, left, ...)?

I think screen padding options make more sense but would be more complicated to implement... 😬

I just did a little experimentation with this PR and discovered that Map#fitBounds always resets the bearing to 0 when it is called. 🤔

We may be able to rename these properties, ship this PR as-is, and open a separate issue about preserving bearing during a Map#fitBounds operation.

mohsen1 · 2017-01-17T20:48:00Z

js/ui/camera.js

     * @see [Fit a map to a bounding box](https://www.mapbox.com/mapbox-gl-js/example/fitbounds/)
     */
    fitBounds(bounds, options, eventData) {

        options = util.extend({
-            padding: 0,
+            padding: {
+                north: 0,


I think it makes more sense to name these top, right, bottom and left. It's more consistent with Leaflet and Google Maps APIs.

I agree. If fitBounds is called on a (potentially) rotated map n, s, e and w don't make sense as you would want the padding to be applied to the viewport, not the feature.

mollymerp · 2017-02-01T03:20:04Z

I updated the approach to one @lucaswoj and I discussed a couple weeks ago where we separate the padding out into two parts: the part that does not alter the center of the map (i.e. equal padding amts on top/bottom and left/right) and the part that does (any left over padding from the first part) and the latter part gets added to the offset object which is handled in subsequent calls to flyTo or easeTo.

So if you pass a padding object like {padding: {top: 10, bottom: 20, right: 50, left: 75}} this will get broken into {lateralPadding: 50, verticalPadding: 10, offset: [25, -10]}

This was a compromise to get accurate results without completely refactoring the camera 😅 -- the same results are achievable with the current API, but hopefully this option will be more clear to users going forward.

mollymerp · 2017-02-01T03:28:11Z

@lucaswoj I also added a typedef for the PaddingOptions, but I kind of think this makes our docs less clear (i.e. users have to click through to find out how to add padding) and since padding is only used in this one function we don't end up saving a ton of space. What do you think?

lucaswoj · 2017-02-01T23:53:47Z

@mollymerp 👍 on the typedef. The typedef is the most semantically correct. I hope we'll have an oppurtunity to refactor when we hit #2801. I also hope we'll add better support for {inline: declarations} soon.

- ref #995

lucaswoj

I'm unsure about the need to support both offset and padding. The relationship between the two is a little confusing. padding alone should be functionally sufficient.

Could we deprecate offset and remove it in a future release?

lucaswoj · 2017-02-08T20:33:50Z

src/ui/camera.js

+            console.log(Object.keys(options.padding).sort((a, b)=> {
+                if (a < b) return -1;
+                if (a > b) return 1;
+                return 0;


accidental console.log?

As best I can tell, these lines are equivalent to .sort((a, b)=> a - b)

Actually, I don't think they're equivalent... according to the docs the approach you suggest can be used for comparing numeric values, but not for strings.

mollymerp · 2017-02-09T00:54:54Z

offset is an AnimationOptions property (should really be a Camera option I think), while padding is only an option on Map#fitBounds. If we deprecate one of them it should be padding I think.

Given that this is one of the most requested features, maybe we should merge and this in advance of a larger refactor of the Camera API + documentation?

anandthakker

Looks great to me, barring a couple of minor suggestions.

Regarding the redundancy between offset and padding: while I agree that the redundancy could be confusing, this is a case where parsimony might be more confusing, because while they're functionally redundant, they represent distinct use cases, and it's hard (or at least annoying) to convert between them.

anandthakker · 2017-02-09T18:20:43Z

src/ui/camera.js

     *
     * @memberof Map#
     * @param {LngLatBoundsLike} bounds Center these bounds in the viewport and use the highest
     *      zoom level up to and including `Map#getMaxZoom()` that fits them in the viewport.
-     * @param {Object} [options]
+     * @param {AnimationOptions | CameraOptions | PaddingOptions} [options]


The way I read this, it makes me think that top, bottom, etc. are properties that can go directly on options. Since these should actually be sub-properties of options.padding, should this be @param {number|PaddingOptions} [options.padding=0] The amount of padding to allow around the given bounds. (or something less stilted-sounding heh)

anandthakker · 2017-02-09T18:26:05Z

test/unit/ui/camera.test.js

+            const bb = [[-133, 16], [-68, 50]];
+
+            camera.fitBounds(bb, {duration:0});
+            t.deepEqual(fixedLngLat(camera.getCenter()), { lng: -100.5, lat: 34.717077774 }, 'pans to coordinates based on fitBounds');


Small preemptive suggestion: should we round the latitude before comparing it?

fixedLngLat does round to a default precision of 9 decimal places – that is rather precise, though, so it might be a good idea to reduce precision a bit

Oh whoops 🙈 totally didn't see that!

lucaswoj · 2017-02-13T19:04:45Z

Per chat with @mollymerp today, we're going to 🚢 this API and look to make simplifying + breaking changes in conjunction with #2801

mollymerp added the under development label Jan 4, 2017

mollymerp changed the title ~~Allow an array to be passed for fitBounds padding option~~ Allow user to specify n, s, e, w padding options for fitBounds Jan 5, 2017

mollymerp added feature 🍏 and removed under development labels Jan 5, 2017

lucaswoj reviewed Jan 9, 2017

View reviewed changes

mohsen1 reviewed Jan 17, 2017

View reviewed changes

lucaswoj added the under development label Jan 17, 2017

mollymerp force-pushed the fitbounds-padding-options branch from fa5ad12 to 53dad67 Compare February 1, 2017 03:00

mollymerp force-pushed the fitbounds-padding-options branch from 8920c87 to 6e2db2b Compare February 1, 2017 03:20

mollymerp mentioned this pull request Feb 1, 2017

Display an error message when fitBounds' padding is larger than map #1776

Closed

tristen and others added 12 commits February 8, 2017 11:25

Support padding positions in fitBounds

ae26a06

- ref #995

add padding to bounds to get correct viewport bounds

4fe8dae

fix fitbounds padding + scale

4cbbfbe

fix padding tests

dc5aebb

fix testing errors

fda8eba

fix coordinate order, change to object literal for padding option

b4a5c9a

update docs

b2aa07b

update camera

9c838c0

change params to screen directions

f754d8b

one way to do things 💭

110a9c6

update padding approach, add error warnings 🙅

759da9d

add PaddingOptions typedef to docs 📝

57be827

mollymerp force-pushed the fitbounds-padding-options branch from 6e2db2b to 57be827 Compare February 8, 2017 19:25

lucaswoj reviewed Feb 8, 2017

View reviewed changes

mollymerp mentioned this pull request Feb 9, 2017

Allow map center to be configurable for all interactions and controls via an offset option #1740

Closed

anandthakker approved these changes Feb 9, 2017

View reviewed changes

Molly Lloyd added 2 commits February 9, 2017 12:11

update docs + tests

368d917

remove unnecessary console.log

63a2013

mollymerp removed the under development label Feb 10, 2017

mollymerp merged commit 74a4456 into master Feb 13, 2017

mollymerp deleted the fitbounds-padding-options branch February 13, 2017 19:36

songololo mentioned this pull request Aug 6, 2018

Add a global "padding" option #4268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to specify n, s, e, w padding options for fitBounds #3890

Allow user to specify n, s, e, w padding options for fitBounds #3890

mollymerp commented Jan 4, 2017 •

edited

Loading

mollymerp commented Jan 5, 2017

lucaswoj Jan 9, 2017

mollymerp Jan 10, 2017

lucaswoj Jan 9, 2017

mollymerp Jan 10, 2017

lucaswoj Jan 17, 2017 •

edited

Loading

mohsen1 Jan 17, 2017

jaapster Jan 18, 2017

mollymerp commented Feb 1, 2017

mollymerp commented Feb 1, 2017

lucaswoj commented Feb 1, 2017 •

edited

Loading

lucaswoj left a comment

lucaswoj Feb 8, 2017

lucaswoj Feb 8, 2017

mollymerp Feb 9, 2017

mollymerp Feb 9, 2017

mollymerp commented Feb 9, 2017

anandthakker left a comment

anandthakker Feb 9, 2017

anandthakker Feb 9, 2017

mollymerp Feb 9, 2017

anandthakker Feb 9, 2017

lucaswoj commented Feb 13, 2017

Allow user to specify n, s, e, w padding options for fitBounds #3890

Allow user to specify n, s, e, w padding options for fitBounds #3890

Conversation

mollymerp commented Jan 4, 2017 • edited Loading

Launch Checklist

mollymerp commented Jan 5, 2017

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

lucaswoj Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp commented Feb 1, 2017

mollymerp commented Feb 1, 2017

lucaswoj commented Feb 1, 2017 • edited Loading

lucaswoj left a comment

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

Choose a reason for hiding this comment

mollymerp commented Feb 9, 2017

anandthakker left a comment

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

Choose a reason for hiding this comment

lucaswoj commented Feb 13, 2017

mollymerp commented Jan 4, 2017 •

edited

Loading

lucaswoj Jan 17, 2017 •

edited

Loading

lucaswoj commented Feb 1, 2017 •

edited

Loading