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

Don't fire sourcedata event with sourceDataType: 'metadata' when a source is removed #5771

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 0 additions & 1 deletion src/style/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,6 @@ class Style extends Evented {
const sourceCache = this.sourceCaches[id];
delete this.sourceCaches[id];
delete this._updatedSources[id];
sourceCache.fire('data', {sourceDataType: 'metadata', dataType:'source', sourceId: id});
sourceCache.setEventedParent(null);
sourceCache.clearTiles();

Expand Down
9 changes: 3 additions & 6 deletions src/ui/control/logo_control.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class LogoControl {
this._container.appendChild(anchor);
this._container.style.display = 'none';

this._map.on('styledata', this._updateLogo);
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll also need to remove this event listener in the onRemove function

this._map.on('sourcedata', this._updateLogo);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are safe to remove this listener all together. Keeping it here and removing the metadata condition causes _updateLogo to be called on every tile load 🙃

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 think this might still be necessary to ensure that the control updates when TileJSON is loaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, thank you! it was working on the debug page because the spritesheet was loading after the tilejson.

this._updateLogo();
return this._container;
Expand All @@ -46,10 +47,8 @@ class LogoControl {
return 'bottom-left';
}

_updateLogo(e: any) {
if (!e || e.sourceDataType === 'metadata') {
this._container.style.display = this._logoRequired() ? 'block' : 'none';
}
_updateLogo() {
this._container.style.display = this._logoRequired() ? 'block' : 'none';
}

_logoRequired() {
Expand All @@ -65,8 +64,6 @@ class LogoControl {

return false;
}

}


module.exports = LogoControl;
141 changes: 81 additions & 60 deletions test/unit/ui/control/logo.test.js
Original file line number Diff line number Diff line change
@@ -1,88 +1,109 @@
'use strict';

const test = require('mapbox-gl-js-test').test;
const VectorTileSource = require('../../../../src/source/vector_tile_source');
const window = require('../../../../src/util/window');
const Map = require('../../../../src/ui/map');

function createMap(logoPosition, logoRequired) {
test('LogoControl appears in bottom-left by default', (t) => {
const container = window.document.createElement('div');
const map = new Map({container});
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-logo'
).length, 1);
t.end();
});

test('LogoControl appears in the position specified by the position option', (t) => {
const container = window.document.createElement('div');
return new Map({
container: container,
const map = new Map({container, logoPosition: 'top-left'});
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-top-left .mapboxgl-ctrl-logo'
).length, 1);
t.end();
});

test('LogoControl is hidden when no sources with the mapbox_logo property exist', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {
'composite': createSource({
minzoom: 1,
maxzoom: 10,
attribution: "Mapbox",
tiles: [
"http://example.com/{z}/{x}/{y}.png"
]
}, logoRequired)
},
sources: {},
layers: []
},
logoPosition: logoPosition || undefined
});
}

function createSource(options, logoRequired) {
const source = new VectorTileSource('id', options, { send: function () {} });
source.onAdd({
transform: { angle: 0, pitch: 0, showCollisionBoxes: false }
});
source.on('error', (e) => {
throw e.error;
});
const logoFlag = "mapbox_logo";
source[logoFlag] = logoRequired === undefined ? true : logoRequired;
return source;
}
test('LogoControl appears in bottom-left by default', (t) => {
const map = createMap();
map.on('load', () => {
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-bottom-left .mapboxgl-ctrl-logo'
).length, 1);
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'none');
t.end();
});
});

test('LogoControl appears in the position specified by the position option', (t) => {
const map = createMap('top-left');
test('LogoControl is shown when a source with the mapbox_logo property is added', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {},
layers: []
},
});
map.on('load', () => {
t.equal(map.getContainer().querySelectorAll(
'.mapboxgl-ctrl-top-left .mapboxgl-ctrl-logo'
).length, 1);
t.end();
map.addSource('source', {
type: 'raster',
tiles: ["http://example.com/{z}/{x}/{y}.png"],
mapbox_logo: true // eslint-disable-line
});
map.once('sourcedata', () => {
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'block');
t.end();
});
});
});

test('LogoControl is not displayed when the mapbox_logo property is false', (t) => {
const map = createMap('top-left', false);
test('LogoControl is visible when a source with the mapbox_logo property exists', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {
source: {
type: 'raster',
tiles: ["http://example.com/{z}/{x}/{y}.png"],
mapbox_logo: true // eslint-disable-line
}
},
layers: []
},
});
map.on('load', () => {
t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-top-left > .mapboxgl-ctrl')[0].style.display, 'none');
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'block');
t.end();
});
});
test('LogoControl is not added more than once', (t)=>{
const map = createMap();
const source = createSource({
minzoom: 1,
maxzoom: 10,
attribution: "Mapbox",
tiles: [
"http://example.com/{z}/{x}/{y}.png"
]

test('LogoControl is hidden when the last source with the mapbox_logo property is removed', (t) => {
const container = window.document.createElement('div');
const map = new Map({
container,
style: {
version: 8,
sources: {
source: {
type: 'raster',
tiles: ["http://example.com/{z}/{x}/{y}.png"],
mapbox_logo: true // eslint-disable-line
}
},
layers: []
},
});
map.on('load', ()=>{
t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-logo').length, 1, 'first LogoControl');
map.addSource('source2', source);
map.on('sourcedata', (e)=>{
if (e.isSourceLoaded && e.sourceId === 'source2' && e.sourceDataType === 'metadata') {
t.equal(map.getContainer().querySelectorAll('.mapboxgl-ctrl-logo').length, 1, 'only one LogoControl is added with multiple sources');
t.end();
}
map.on('load', () => {
map.removeSource('source');
map.once('styledata', () => {
t.equal(map.getContainer().querySelector('.mapboxgl-ctrl-logo').parentNode.style.display, 'none');
t.end();
});
});
});