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

geo.visible false should override template.layout.geo.show* #4483

Merged
merged 7 commits into from
Jan 13, 2020
23 changes: 23 additions & 0 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -1446,6 +1446,29 @@ plots.supplyLayoutGlobalDefaults = function(layoutIn, layoutOut, formatObj) {

var template = layoutIn.template;
if(Lib.isPlainObject(template)) {
if( // geo.visible false should override template.layout.geo.show* - see issue 4482
layoutIn.geo &&
layoutIn.geo.visible === false &&
template.layout &&
template.layout.geo
alexcjohnson marked this conversation as resolved.
Show resolved Hide resolved
) {
// make a copy
var newTemplate = Lib.extendDeep({}, template);

// override show*
newTemplate.layout.geo.showcoastlines = false;
newTemplate.layout.geo.showcountries = false;
newTemplate.layout.geo.showframe = false;
newTemplate.layout.geo.showlakes = false;
newTemplate.layout.geo.showland = false;
newTemplate.layout.geo.showocean = false;
newTemplate.layout.geo.showrivers = false;
newTemplate.layout.geo.showsubunits = false;

// set ref to copy
template = newTemplate;
}

layoutOut.template = template;
layoutOut._template = template.layout;
layoutOut._dataTemplate = template.data;
Expand Down
45 changes: 45 additions & 0 deletions test/jasmine/tests/geo_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,51 @@ describe('Test Geo layout defaults', function() {
});
});

describe('geo visible false', function() {
var gd;

beforeEach(function() { gd = createGraphDiv(); });

afterEach(destroyGraphDiv);

it('should override template.layout.geo.show*', function(done) {
var keys = [
'showcoastlines',
'showcountries',
'showframe',
'showland',
'showlakes',
'showocean',
'showrivers',
'showsubunits'
];

var layout = {
geo: { visible: false },
template: {
layout: {
geo: {}
}
}
};

keys.forEach(function(k) {
layout.template.layout.geo[k] = true;
});

var data = [{ lon: [0], lat: [0], type: 'scattergeo' }];

Plotly.plot(gd, data, layout)
.then(function() {
keys.forEach(function(k) {
expect(gd._fullLayout.template.layout.geo[k]).toBe(false, k);
Copy link
Collaborator

Choose a reason for hiding this comment

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

At the end of the day we don't care what's in _fullLayout.template - only the values of _fullLayout.geo*[k]. Would you mind modifying the test to look at these attributes, under both of the cases we discussed: visible: false in the main layout as well as visible: false in the template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Fixed by 7e62ce4.

});
})
.catch(failTest)
.then(done);
});
});

describe('geojson / topojson utils', function() {
function _locationToFeature(topojson, loc, locationmode) {
var trace = { locationmode: locationmode };
Expand Down