From 86e18fb0e646edcfb66e01f36b07d1ff14a1fcbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 18 Sep 2018 14:54:31 -0400 Subject: [PATCH 1/5] include 'trace' layout attributes set in subplot containers ... during validation. This solution is not-strict as it could be, but at least fixes the false-negative case of #3022. --- src/plot_api/validate.js | 15 ++++++++++---- test/jasmine/tests/validate_test.js | 32 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/plot_api/validate.js b/src/plot_api/validate.js index dc77673cc74..995d7a4aa33 100644 --- a/src/plot_api/validate.js +++ b/src/plot_api/validate.js @@ -281,16 +281,23 @@ function crawl(objIn, objOut, schema, list, base, path) { // the 'full' layout schema depends on the traces types presents function fillLayoutSchema(schema, dataOut) { + var layoutSchema = schema.layout.layoutAttributes; + for(var i = 0; i < dataOut.length; i++) { - var traceType = dataOut[i].type, - traceLayoutAttr = schema.traces[traceType].layoutAttributes; + var traceOut = dataOut[i]; + var traceSchema = schema.traces[traceOut.type]; + var traceLayoutAttr = traceSchema.layoutAttributes; if(traceLayoutAttr) { - Lib.extendFlat(schema.layout.layoutAttributes, traceLayoutAttr); + if(traceOut.subplot) { + Lib.extendFlat(layoutSchema[traceSchema.attributes.subplot.dflt], traceLayoutAttr); + } else { + Lib.extendFlat(layoutSchema, traceLayoutAttr); + } } } - return schema.layout.layoutAttributes; + return layoutSchema; } // validation error codes diff --git a/test/jasmine/tests/validate_test.js b/test/jasmine/tests/validate_test.js index 0c1074fd250..6cac39770b1 100644 --- a/test/jasmine/tests/validate_test.js +++ b/test/jasmine/tests/validate_test.js @@ -539,4 +539,36 @@ describe('Plotly.validate', function() { var out = Plotly.validate(shapeMock.data, shapeMock.layout); expect(out).toBeUndefined(); }); + + it('should work with *trace* layout attributes', function() { + var out = Plotly.validate([{ + type: 'bar', + y: [1, 2, 1] + }, { + type: 'barpolar', + r: [1, 2, 3] + }, { + type: 'scatterpolar', + theta: [0, 90, 200], + subplot: 'polar2' + }], { + bargap: 0.3, + polar: {bargap: 0.2}, + polar2: {bargap: 0.05}, + polar3: {bargap: 0.4} + }); + + expect(out.length).toBe(1); + assertErrorContent( + out[0], 'unused', 'layout', null, ['polar3'], 'polar3', + 'In layout, container polar3 did not get coerced' + ); + + // Plotly.validate should be more strict here. + // + // It should log an 'unused' warning for `polar2.bargap`, + // but trace layout attribute set in subplot containers are considered + // valid as long as one trace on that graph has those trace layout + // attributes. + }); }); From c5cf89b71cd091a5b742fb2a80618672cce34cc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 21 Sep 2018 12:08:45 -0400 Subject: [PATCH 2/5] make supplyDefault (and Plotly.validate) more strict w/ polar?.bar* - coerce polar?.bargap and polar?.barmode only for subplots with visible barpolar traces on them. --- src/traces/barpolar/layout_defaults.js | 18 +++++++++----- test/jasmine/tests/barpolar_test.js | 33 ++++++++++++++++++++++++++ test/jasmine/tests/validate_test.js | 15 +++++------- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/traces/barpolar/layout_defaults.js b/src/traces/barpolar/layout_defaults.js index 0a2f011f2f8..ed4501933fa 100644 --- a/src/traces/barpolar/layout_defaults.js +++ b/src/traces/barpolar/layout_defaults.js @@ -11,17 +11,23 @@ var Lib = require('../../lib'); var attrs = require('./layout_attributes'); -module.exports = function(layoutIn, layoutOut) { - var subplotIds = layoutOut._subplots.polar; +module.exports = function(layoutIn, layoutOut, fullData) { + var subplotsDone = {}; var sp; function coerce(attr, dflt) { return Lib.coerce(layoutIn[sp] || {}, layoutOut[sp], attrs, attr, dflt); } - for(var i = 0; i < subplotIds.length; i++) { - sp = subplotIds[i]; - coerce('barmode'); - coerce('bargap'); + for(var i = 0; i < fullData.length; i++) { + var trace = fullData[i]; + if(trace.type === 'barpolar' && trace.visible === true) { + sp = trace.subplot; + if(!subplotsDone[sp]) { + coerce('barmode'); + coerce('bargap'); + subplotsDone[sp] = 1; + } + } } }; diff --git a/test/jasmine/tests/barpolar_test.js b/test/jasmine/tests/barpolar_test.js index b5314a8f330..4fc26147782 100644 --- a/test/jasmine/tests/barpolar_test.js +++ b/test/jasmine/tests/barpolar_test.js @@ -1,10 +1,43 @@ var Plotly = require('@lib'); var Lib = require('@src/lib'); +var supplyAllDefaults = require('../assets/supply_defaults'); var createGraphDiv = require('../assets/create_graph_div'); var destroyGraphDiv = require('../assets/destroy_graph_div'); var failTest = require('../assets/fail_test'); +describe('Test barpolar defaults:', function() { + var gd; + + function _supply(opts, layout) { + gd = {}; + opts = Array.isArray(opts) ? opts : [opts]; + + gd.data = opts.map(function(o) { + return Lib.extendFlat({type: 'barpolar'}, o || {}); + }); + gd.layout = layout || {}; + + supplyAllDefaults(gd); + } + + it('should not coerce polar.bar* attributes on subplot w/o visible barpolar', function() { + _supply([ + {visible: false, subplot: 'polar'}, + {r: [1], subplot: 'polar2'}, + {type: 'scatterpolar', r: [1], subplot: 'polar3'} + ]); + + var fullLayout = gd._fullLayout; + expect(fullLayout.polar.barmode).toBeUndefined(); + expect(fullLayout.polar.bargap).toBeUndefined(); + expect(fullLayout.polar2.barmode).toBe('stack'); + expect(fullLayout.polar2.bargap).toBe(0.1); + expect(fullLayout.polar3.barmode).toBeUndefined(); + expect(fullLayout.polar3.bargap).toBeUndefined(); + }); +}); + describe('Test barpolar hover:', function() { var gd; diff --git a/test/jasmine/tests/validate_test.js b/test/jasmine/tests/validate_test.js index 6cac39770b1..d3123bd3a1a 100644 --- a/test/jasmine/tests/validate_test.js +++ b/test/jasmine/tests/validate_test.js @@ -558,17 +558,14 @@ describe('Plotly.validate', function() { polar3: {bargap: 0.4} }); - expect(out.length).toBe(1); + expect(out.length).toBe(2); assertErrorContent( - out[0], 'unused', 'layout', null, ['polar3'], 'polar3', + out[0], 'unused', 'layout', null, ['polar2', 'bargap'], 'polar2.bargap', + 'In layout, key polar2.bargap did not get coerced' + ); + assertErrorContent( + out[1], 'unused', 'layout', null, ['polar3'], 'polar3', 'In layout, container polar3 did not get coerced' ); - - // Plotly.validate should be more strict here. - // - // It should log an 'unused' warning for `polar2.bargap`, - // but trace layout attribute set in subplot containers are considered - // valid as long as one trace on that graph has those trace layout - // attributes. }); }); From 1c0d3ed0512ef826bf2e6dc9d576bccfff2f18f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 21 Sep 2018 12:14:13 -0400 Subject: [PATCH 3/5] add deprecation warning to legacy polar charts attr descriptions --- src/plots/polar/legacy/axis_attributes.js | 25 ++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/plots/polar/legacy/axis_attributes.js b/src/plots/polar/legacy/axis_attributes.js index 3a6d899a198..d2f0e0ca534 100644 --- a/src/plots/polar/legacy/axis_attributes.js +++ b/src/plots/polar/legacy/axis_attributes.js @@ -13,6 +13,8 @@ var axesAttrs = require('../../cartesian/layout_attributes'); var extendFlat = require('../../../lib/extend').extendFlat; var overrideAll = require('../../../plot_api/edit_types').overrideAll; +var deprecationWarning = 'Legacy polar charts are deprecated!'; + var domainAttr = extendFlat({}, axesAttrs.domain, { description: [ 'Polar chart subplots are not supported yet.', @@ -26,6 +28,7 @@ function mergeAttrs(axisName, nonCommonAttrs) { valType: 'boolean', role: 'style', description: [ + deprecationWarning, 'Determines whether or not the line bounding this', axisName, 'axis', 'will be shown on the figure.' @@ -35,6 +38,7 @@ function mergeAttrs(axisName, nonCommonAttrs) { valType: 'boolean', role: 'style', description: [ + deprecationWarning, 'Determines whether or not the', axisName, 'axis ticks', 'will feature tick labels.' @@ -45,6 +49,7 @@ function mergeAttrs(axisName, nonCommonAttrs) { values: ['horizontal', 'vertical'], role: 'style', description: [ + deprecationWarning, 'Sets the orientation (from the paper perspective)', 'of the', axisName, 'axis tick labels.' ].join(' ') @@ -54,6 +59,7 @@ function mergeAttrs(axisName, nonCommonAttrs) { min: 0, role: 'style', description: [ + deprecationWarning, 'Sets the length of the tick lines on this', axisName, 'axis.' ].join(' ') }, @@ -61,6 +67,7 @@ function mergeAttrs(axisName, nonCommonAttrs) { valType: 'color', role: 'style', description: [ + deprecationWarning, 'Sets the color of the tick lines on this', axisName, 'axis.' ].join(' ') }, @@ -68,17 +75,20 @@ function mergeAttrs(axisName, nonCommonAttrs) { valType: 'string', role: 'style', description: [ + deprecationWarning, 'Sets the length of the tick lines on this', axisName, 'axis.' ].join(' ') }, endpadding: { valType: 'number', - role: 'style' + role: 'style', + description: deprecationWarning, }, visible: { valType: 'boolean', role: 'info', description: [ + deprecationWarning, 'Determines whether or not this axis will be visible.' ].join(' ') } @@ -97,6 +107,7 @@ module.exports = overrideAll({ { valType: 'number' } ], description: [ + deprecationWarning, 'Defines the start and end point of this radial axis.' ].join(' ') }, @@ -105,6 +116,7 @@ module.exports = overrideAll({ valType: 'number', role: 'style', description: [ + deprecationWarning, 'Sets the orientation (an angle with respect to the origin)', 'of the radial axis.' ].join(' ') @@ -120,6 +132,7 @@ module.exports = overrideAll({ { valType: 'number', dflt: 360 } ], description: [ + deprecationWarning, 'Defines the start and end point of this angular axis.' ].join(' ') }, @@ -133,16 +146,18 @@ module.exports = overrideAll({ values: ['clockwise', 'counterclockwise'], role: 'info', description: [ - 'For polar plots only.', - 'Sets the direction corresponding to positive angles.' + deprecationWarning, + 'Sets the direction corresponding to positive angles', + 'in legacy polar charts.' ].join(' ') }, orientation: { valType: 'angle', role: 'info', description: [ - 'For polar plots only.', - 'Rotates the entire polar by the given angle.' + deprecationWarning, + 'Rotates the entire polar by the given angle', + 'in legacy polar charts.' ].join(' ') } } From f32b398390bf67f74fdf481bfc432a8e40509ec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 21 Sep 2018 12:18:01 -0400 Subject: [PATCH 4/5] add deprecation warning to area traces attr desciptions --- src/plots/polar/legacy/area_attributes.js | 45 ++++++++++++++++++++--- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/plots/polar/legacy/area_attributes.js b/src/plots/polar/legacy/area_attributes.js index 4ab7ae76f87..e13078ec4ff 100644 --- a/src/plots/polar/legacy/area_attributes.js +++ b/src/plots/polar/legacy/area_attributes.js @@ -10,15 +10,48 @@ var scatterAttrs = require('../../../traces/scatter/attributes'); var scatterMarkerAttrs = scatterAttrs.marker; +var extendFlat = require('../../../lib/extend').extendFlat; + +var deprecationWarning = 'Area traces are deprecated!'; module.exports = { - r: scatterAttrs.r, - t: scatterAttrs.t, + r: extendFlat({}, scatterAttrs.r, { + description: [ + deprecationWarning, + scatterAttrs.r.description + ].join(' ') + }), + t: extendFlat({}, scatterAttrs.t, { + description: [ + deprecationWarning, + scatterAttrs.t.description + ].join(' ') + }), marker: { - color: scatterMarkerAttrs.color, - size: scatterMarkerAttrs.size, - symbol: scatterMarkerAttrs.symbol, - opacity: scatterMarkerAttrs.opacity, + color: extendFlat({}, scatterMarkerAttrs.color, { + description: [ + deprecationWarning, + scatterMarkerAttrs.color.description + ].join(' ') + }), + size: extendFlat({}, scatterMarkerAttrs.size, { + description: [ + deprecationWarning, + scatterMarkerAttrs.size.description + ].join(' ') + }), + symbol: extendFlat({}, scatterMarkerAttrs.symbol, { + description: [ + deprecationWarning, + scatterMarkerAttrs.symbol.description + ].join(' ') + }), + opacity: extendFlat({}, scatterMarkerAttrs.opacity, { + description: [ + deprecationWarning, + scatterMarkerAttrs.opacity.description + ].join(' ') + }), editType: 'calc' } }; From 019bacfc88069f51133f624c49063e5b01c94d58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 21 Sep 2018 14:04:15 -0400 Subject: [PATCH 5/5] AJ-proof legacy polar depreaction warnings --- src/plots/polar/legacy/area_attributes.js | 11 ++++++++--- src/plots/polar/legacy/axis_attributes.js | 5 ++++- src/traces/scatter/attributes.js | 14 ++++++++------ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/plots/polar/legacy/area_attributes.js b/src/plots/polar/legacy/area_attributes.js index e13078ec4ff..1cedc2fc96d 100644 --- a/src/plots/polar/legacy/area_attributes.js +++ b/src/plots/polar/legacy/area_attributes.js @@ -12,19 +12,24 @@ var scatterAttrs = require('../../../traces/scatter/attributes'); var scatterMarkerAttrs = scatterAttrs.marker; var extendFlat = require('../../../lib/extend').extendFlat; -var deprecationWarning = 'Area traces are deprecated!'; +var deprecationWarning = [ + 'Area traces are deprecated!', + 'Please switch to the *barpolar* trace type.' +].join(' '); module.exports = { r: extendFlat({}, scatterAttrs.r, { description: [ deprecationWarning, - scatterAttrs.r.description + 'Sets the radial coordinates', + 'for legacy polar chart only.' ].join(' ') }), t: extendFlat({}, scatterAttrs.t, { description: [ deprecationWarning, - scatterAttrs.t.description + 'Sets the angular coordinates', + 'for legacy polar chart only.' ].join(' ') }), marker: { diff --git a/src/plots/polar/legacy/axis_attributes.js b/src/plots/polar/legacy/axis_attributes.js index d2f0e0ca534..79addb4ca3f 100644 --- a/src/plots/polar/legacy/axis_attributes.js +++ b/src/plots/polar/legacy/axis_attributes.js @@ -13,7 +13,10 @@ var axesAttrs = require('../../cartesian/layout_attributes'); var extendFlat = require('../../../lib/extend').extendFlat; var overrideAll = require('../../../plot_api/edit_types').overrideAll; -var deprecationWarning = 'Legacy polar charts are deprecated!'; +var deprecationWarning = [ + 'Legacy polar charts are deprecated!', + 'Please switch to *polar* subplots.' +].join(' '); var domainAttr = extendFlat({}, axesAttrs.domain, { description: [ diff --git a/src/traces/scatter/attributes.js b/src/traces/scatter/attributes.js index d5359e90f75..b865858ad66 100644 --- a/src/traces/scatter/attributes.js +++ b/src/traces/scatter/attributes.js @@ -544,18 +544,20 @@ module.exports = { valType: 'data_array', editType: 'calc', description: [ - 'For legacy polar chart only.', - 'Please switch to *scatterpolar* trace type.', - 'Sets the radial coordinates.' + 'r coordinates in scatter traces are deprecated!', + 'Please switch to the *scatterpolar* trace type.', + 'Sets the radial coordinates', + 'for legacy polar chart only.' ].join('') }, t: { valType: 'data_array', editType: 'calc', description: [ - 'For legacy polar chart only.', - 'Please switch to *scatterpolar* trace type.', - 'Sets the angular coordinates.' + 't coordinates in scatter traces are deprecated!', + 'Please switch to the *scatterpolar* trace type.', + 'Sets the angular coordinates', + 'for legacy polar chart only.' ].join('') } };