Fix #7517 - asymmetrical cameraForBounds #7518
Conversation
|
The existing test was incorrect given my understanding of how the method should work based on documentation. I fixed the test and added more to explicitly test these scenarios. |
|
I added a debug file to test the behavior visually. I also used it as a fiddle: |
|
Linter now passes. Ready for review 👍 |
src/ui/camera.js
Outdated
|
|
||
| // Calculate center: apply the zoom, the configured offset, as well as offset that exists as a result of padding. | ||
| const paddingOffset = [(options.padding.left - options.padding.right) / 2, (options.padding.top - options.padding.bottom) / 2]; | ||
| const offsetAtInitialZoom = Point.convert([options.offset[0] + paddingOffset[0], options.offset[1] + paddingOffset[1]]); |
There was a problem hiding this comment.
Nitpick: let's avoid temporary arrays here by using new Point(x, y) instead of Point.convert([x, y]), and maybe declaring separate paddingOffsetX and paddingOffsetY variables since we don't really need it being in an array.
There was a problem hiding this comment.
Also, options.offset is of PointLike type so this code will break if you pass e.g. offset: new Point(10, 10) instead of offset: [10, 10]. So let's bring back the const offset = Point.convert(options.offset) line and use that.
There was a problem hiding this comment.
Great points! It looks like the previous implementation was also accessing array indices directly, so I added a test to make sure PointLike objects work.
|
Awesome, thank you! |
The
_cameraForBoxAndBearingmethod had several shortcomings that causedcameraForBoundsto return incorrect values whenpaddingoroffsetare asymmetrical. Additionally, because the documented return value is aCameraOptionsobject, we can't rely on theoffsetproperty which was leaked through in the previous implementation.Launch Checklist