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

account for padding in getBounds #10386

Merged
merged 5 commits into from
Feb 17, 2021
Merged

account for padding in getBounds #10386

merged 5 commits into from
Feb 17, 2021

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Feb 16, 2021

Launch Checklist

  • briefly describe the changes in this PR

This change proposes to alter map.getBounds(), when a global map padding is set, to return the inset bounds rather than the outset bounds (current behaviour).

This probably needs some discussion, not sure if it's considered a breaking change or a bug fix, however by making this the new default it ensures that:

  1. box zoom interaction ensures that all the area within the selected box is visible within the inset area (should address boxZoom with padding #9056)
  2. getBounds followed by fitBounds ensures the map view doesn't change (should address getBounds value is wrong when I set fitBounds with padding #10169)

My rationale is once you've set your global map padding to account for UI elements then all bounds refer to the inset bounds for consistency.

We could consider adding an option to getBounds/fitBounds to specify if the bounds should be for the inset or outset padded area, as I can see times when a dev would like to specify this.

  • include before/after visuals or gifs if this PR includes visual changes

before

getBounds:
Screenshot from 2021-02-16 13-48-33

setBounds:
Screenshot from 2021-02-16 13-48-46

after:

getBounds/setBounds:
Screenshot from 2021-02-16 13-47-43

  • write tests for all new functionality

added one unit test to confirm that getBounds returns the inset when padding is set

  • document any changes to public APIs
    I've updated the docs for getBounds/fitBounds to specify it's for the inset when padding is set, however I can see especially with fitBounds how someone reading the docs could get confused between the two different kinds of padding (map.setPadding and map.fitBounds padding option), not sure how to improve that.

  • post benchmark scores

  • manually test the debug page
    yes, I added a new debug page

  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
    feature or bug? Possibly considered a breaking change.

  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Changed map.getBounds() to return the inset bounds when a map padding is set</changelog>

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Personally I think this makes a lot of sense, and is more of a bug fix than breaking behavior. Padding behavior will be much less confusing with this change. Thanks a lot for working on this!

@andrewharvey andrewharvey marked this pull request as ready for review February 16, 2021 21:32
@mourner
Copy link
Member

mourner commented Feb 17, 2021

@andrewharvey the render test failures are unrelated — need a rebase on latest main to fix.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

👍

@mourner mourner merged commit 9a08dd4 into main Feb 17, 2021
@mourner mourner deleted the getbounds-padding branch February 17, 2021 15:45
@karimnaaji karimnaaji added this to the v2.2 milestone Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants