geo.visible false should override template.layout.geo.show* #4483
Conversation
test/jasmine/tests/geo_test.js
Outdated
| Plotly.plot(gd, data, layout) | ||
| .then(function() { | ||
| keys.forEach(function(k) { | ||
| expect(gd._fullLayout.template.layout.geo[k]).toBe(false, k); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good call. Fixed by 7e62ce4.
|
You’re doing the axis grid show attrs too right? |
Oh no! I misread your reply here: #4482 (comment) Thanks for pointing out! |
Now done in 1b905a1. |
src/plots/geo/layout_defaults.js
Outdated
| @@ -46,6 +46,28 @@ function handleGeoDefaults(geoLayoutIn, geoLayoutOut, coerce, opts) { | |||
| var isClipped = geoLayoutOut._isClipped = !!constants.lonaxisSpan[projType]; | |||
|
|
|||
| var visible = coerce('visible'); | |||
| if(visible === false) { | |||
There was a problem hiding this comment.
I think this is still incorrect when the visible=false came from the template (which we still need a test for - showing that all the show* end up true in that case). I guess the condition here should be if(geoLayoutIn.visible === false)
There was a problem hiding this comment.
Addressed in 639913e.
src/plots/geo/layout_defaults.js
Outdated
| @@ -46,6 +46,29 @@ function handleGeoDefaults(geoLayoutIn, geoLayoutOut, coerce, opts) { | |||
| var isClipped = geoLayoutOut._isClipped = !!constants.lonaxisSpan[projType]; | |||
|
|
|||
| var visible = coerce('visible'); | |||
| if(geoLayoutIn.visible === false) { | |||
| visible = geoLayoutOut.visible = true; | |||
There was a problem hiding this comment.
Why are we setting geoLayoutOut.visible = true here? Looks to me as though it doesn't matter for the end result, is that right? RCE would want the false to remain intact.
| layoutIn = { | ||
| template: { | ||
| layout: { | ||
| geo: { |
There was a problem hiding this comment.
sorry to be a pain, but I still don't see a test case that sets template.layout.geo.visible=false, does NOT set layout.geo.visible, and verifies that template.layout.geo.show* settings are still honored in fullLayout.geo
when template.layout.geo.visible is set to false, and does NOT set layout.geo.visible template
|
Reminder: we need this merged and released by end of day today if at all possible :) |
Fixes: #4482 | before vs after
@plotly/plotly_js
cc: @nicolaskruchten @alexcjohnson