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

add initial bounds as map constructor option #1970 #5518

Merged
merged 5 commits into from
Oct 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/ui/map.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ const defaultOptions = {
* @param {number} [options.zoom=0] The initial zoom level of the map. If `zoom` is not specified in the constructor options, Mapbox GL JS will look for it in the map's style object. If it is not specified in the style, either, it will default to `0`.
* @param {number} [options.bearing=0] The initial bearing (rotation) of the map, measured in degrees counter-clockwise from north. If `bearing` is not specified in the constructor options, Mapbox GL JS will look for it in the map's style object. If it is not specified in the style, either, it will default to `0`.
* @param {number} [options.pitch=0] The initial pitch (tilt) of the map, measured in degrees away from the plane of the screen (0-60). If `pitch` is not specified in the constructor options, Mapbox GL JS will look for it in the map's style object. If it is not specified in the style, either, it will default to `0`.
* @param {LngLatBoundsLike} [options.bounds] The initial bounds of the map. If `bounds` is specified, it overrides `center` and `zoom` constructor options.
* @param {boolean} [options.renderWorldCopies=true] If `true`, multiple copies of the world will be rendered, when zoomed out.
* @param {number} [options.maxTileCacheSize=null] The maximum number of tiles stored in the tile cache for a given source. If omitted, the cache will be dynamically sized based on the current viewport.
* @param {string} [options.localIdeographFontFamily=null] If specified, defines a CSS font-family
Expand Down Expand Up @@ -369,12 +370,17 @@ class Map extends Camera {
this._hash = options.hash && (new Hash()).addTo(this);
// don't set position from options if set through hash
if (!this._hash || !this._hash._onHashChange()) {
this.jumpTo({
center: options.center,
zoom: options.zoom,
bearing: options.bearing,
pitch: options.pitch
});
if (options.bounds) {
this.resize();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the resize here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without resize, Transform's pixelMatrixInverse is undefined here:

vec4.transformMat4(coord0, coord0, this.pixelMatrixInverse);

Maybe we can move following resize call before condition?

this.resize();

Copy link
Member

Choose a reason for hiding this comment

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

@stepankuzmin yes, let's do that — seems to be more logical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

this.fitBounds(options.bounds, { duration: 0 });
} else {
this.jumpTo({
center: options.center,
zoom: options.zoom,
bearing: options.bearing,
pitch: options.pitch
});
}
}

this.resize();
Expand Down
14 changes: 14 additions & 0 deletions test/unit/ui/map.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,20 @@ test('Map', (t) => {
t.end();
});

t.test('initial bounds in constructor options', (t) => {
const container = window.document.createElement('div');
Object.defineProperty(container, 'offsetWidth', {value: 512});
Object.defineProperty(container, 'offsetHeight', {value: 512});

const bounds = [[-133, 16], [-68, 50]];
const map = createMap(t, {container, bounds});

t.deepEqual(fixedLngLat(map.getCenter(), 4), { lng: -100.5, lat: 34.7171 });
t.equal(fixedNum(map.getZoom(), 3), 2.113);

t.end();
});

Copy link
Member

Choose a reason for hiding this comment

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

But the same steps reproduced in test somehow outcomes with different zoom level

The zoom level depends on the size of the map container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/ui/camera.test.js#L1355 as an example. And it works fine with the zoom level.

t.test('disables handlers', (t) => {
t.test('disables all handlers', (t) => {
const map = createMap(t, {interactive: false});
Expand Down