From f55e58dc8b6198ed6e0f123261e500d4741316f9 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 11:32:20 -0500 Subject: [PATCH 1/8] Switch groupby tests to failing syntax --- test/jasmine/tests/transform_groupby_test.js | 66 ++++++++++++++------ 1 file changed, 48 insertions(+), 18 deletions(-) diff --git a/test/jasmine/tests/transform_groupby_test.js b/test/jasmine/tests/transform_groupby_test.js index bb2ea0f607e..9144a4ade01 100644 --- a/test/jasmine/tests/transform_groupby_test.js +++ b/test/jasmine/tests/transform_groupby_test.js @@ -19,7 +19,10 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [ + {target: 'a', value: {marker: {color: 'red'}}}, + {target: 'b', value: {marker: {color: 'blue'}}} + ] }] }]; @@ -30,7 +33,10 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: ['b', 'a', 'b', 'b', 'b', 'a', 'a'], - style: { a: {marker: {color: 'green'}}, b: {marker: {color: 'black'}} } + style: [ + {target: 'a', value: {marker: {color: 'green'}}}, + {target: 'b', value: {marker: {color: 'black'}}} + ] }] }]; @@ -92,7 +98,10 @@ describe('groupby', function() { expect(gd._fullData[1].marker.opacity).toEqual(1); return Plotly.restyle(gd, { - 'transforms[0].style': { a: {marker: {color: 'green'}}, b: {marker: {color: 'red'}} }, + 'transforms[0].style': [ + {target: 'a', value: {marker: {color: 'green'}}}, + {target: 'b', value: {marker: {color: 'red'}}} + ], 'marker.opacity': 0.4 }); }).then(function() { @@ -192,7 +201,10 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [ + {target: 'a', value: {marker: {color: 'red'}}}, + {target: 'b', value: {marker: {color: 'blue'}}} + ] }] }]; @@ -387,7 +399,10 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [ + {target: 'a', value: {marker: {color: 'red'}}}, + {taret: 'b', value: {marker: {color: 'blue'}}} + ] }] }]; @@ -401,8 +416,9 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { - a: { + style: [{ + target: 'a', + value: { marker: { color: 'orange', size: 20, @@ -410,8 +426,10 @@ describe('groupby', function() { color: 'red' } } - }, - b: { + } + }, { + target: 'b', + value: { mode: 'markers+lines', // heterogeonos attributes are OK: group 'a' doesn't need to define this marker: { color: 'cyan', @@ -426,7 +444,7 @@ describe('groupby', function() { color: 'purple' } } - } + }] }] }]; @@ -447,11 +465,14 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { - a: {marker: {size: 30}}, + style: [{ + target: 'a', + value: {marker: {size: 30}} + }, { // override general color: - b: {marker: {size: 15, line: {color: 'yellow'}}, line: {color: 'purple'}} - } + target: 'b', + value: {marker: {size: 15, line: {color: 'yellow'}}, line: {color: 'purple'}} + }] }] }]; @@ -464,7 +485,7 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: {/* can be empty, or of partial group id coverage */} + style: [/* can be empty, or of partial group id coverage */] }] }]; @@ -548,7 +569,10 @@ describe('groupby', function() { transforms: [{ type: 'groupby', // groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [ + {target: 'a', value: {marker: {color: 'red'}}}, + {target: 'b', value: {marker: {color: 'blue'}}} + ] }] }]; @@ -561,7 +585,10 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: [], - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [ + {target: 'a', value: {marker: {color: 'red'}}}, + {target: 'b', value: {marker: {color: 'blue'}}} + ] }] }]; @@ -574,7 +601,10 @@ describe('groupby', function() { transforms: [{ type: 'groupby', groups: null, - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [ + {target: 'a', value: {marker: {color: 'red'}}}, + {target: 'b', value: {marker: {color: 'blue'}}} + ] }] }]; From 67058c919777d90cad2319386e3e48d0bd39373d Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 12:11:42 -0500 Subject: [PATCH 2/8] Rework groupby styles into array of objects --- src/transforms/groupby.js | 64 ++++++++++++++---- test/jasmine/tests/transform_groupby_test.js | 69 +++++++++++++++++++- 2 files changed, 117 insertions(+), 16 deletions(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 0cd744529ca..a5668781a79 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -35,14 +35,24 @@ exports.attributes = { ].join(' ') }, style: { - valType: 'any', - dflt: {}, - description: [ - 'Sets each group style.', - 'For example, with `groups` set to *[\'a\', \'b\', \'a\', \'b\']*', - 'and `style` set to *{ a: { marker: { color: \'red\' } }}', - 'marker points in group *\'a\'* will be drawn in red.' - ].join(' ') + target: { + valType: 'any', + role: 'info', + description: [ + 'The group value which receives these styles.' + ].join(' ') + }, + value: { + valType: 'any', + role: 'info', + dflt: {}, + description: [ + 'Sets each group style.', + 'For example, with `groups` set to *[\'a\', \'b\', \'a\', \'b\']*', + 'and `style` set to *{ a: { marker: { color: \'red\' } }}', + 'marker points in group *\'a\'* will be drawn in red.' + ].join(' ') + }, } }; @@ -71,11 +81,27 @@ exports.supplyDefaults = function(transformIn) { if(!enabled) return transformOut; coerce('groups'); - coerce('style'); + + var styleIn = transformIn.style; + var styleOut = transformOut.style = []; + + if(Array.isArray(styleIn)) { + // If styles are an array, then sanitize them against the schema: + for(var i = 0; i < styleIn.length; i++) { + styleOut[i] = {}; + Lib.coerce(styleIn[i], styleOut[i], exports.attributes.style, 'target'); + Lib.coerce(styleIn[i], styleOut[i], exports.attributes.style, 'value'); + } + } else { + // Otherwise, this is deprecated keyed-object-style styles and we + // just pass the object through as the index itself: + transformOut.style = transformIn.style; + } return transformOut; }; + /** * Apply transform !!! * @@ -115,6 +141,7 @@ function pasteArray(newTrace, trace, j, a) { } function transformOne(trace, state) { + var i; var opts = state.transform; var groups = trace.transforms[state.transformIndex].groups; @@ -128,9 +155,20 @@ function transformOne(trace, state) { var arrayAttrs = PlotSchema.findArrayAttributes(trace); - var style = opts.style || {}; + var style = opts.style || []; + var styleIndex = style; + + // If the styles are an array, then translate it into an index keyed on the + // target. Otherwise just use the object provided for the sake of backward + // compatibility. + if(Array.isArray(style)) { + styleIndex = {}; + for(i = 0; i < style.length; i++) { + styleIndex[style[i].target] = style[i].value; + } + } - for(var i = 0; i < groupNames.length; i++) { + for(i = 0; i < groupNames.length; i++) { var groupName = groupNames[i]; var newTrace = newData[i] = Lib.extendDeepNoArrays({}, trace); @@ -145,9 +183,9 @@ function transformOne(trace, state) { newTrace.name = groupName; - // there's no need to coerce style[groupName] here + // there's no need to coerce styleIndex[groupName] here // as another round of supplyDefaults is done on the transformed traces - newTrace = Lib.extendDeepNoArrays(newTrace, style[groupName] || {}); + newTrace = Lib.extendDeepNoArrays(newTrace, styleIndex[groupName] || {}); } return newData; diff --git a/test/jasmine/tests/transform_groupby_test.js b/test/jasmine/tests/transform_groupby_test.js index 9144a4ade01..b2c4c9f965c 100644 --- a/test/jasmine/tests/transform_groupby_test.js +++ b/test/jasmine/tests/transform_groupby_test.js @@ -64,6 +64,69 @@ describe('groupby', function() { }); }); + it('Accepts deprecated object notation for styles', function(done) { + var oldStyleMockData = [{ + mode: 'markers', + x: [1, -1, -2, 0, 1, 2, 3], + y: [1, 2, 3, 1, 2, 3, 1], + transforms: [{ + type: 'groupby', + groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], + style: { + a: {marker: {color: 'red'}}, + b: {marker: {color: 'blue'}} + } + }] + }]; + var data = Lib.extendDeep([], oldStyleMockData); + data[0].marker = { size: 20 }; + + var gd = createGraphDiv(); + var dims = [4, 3]; + + Plotly.plot(gd, data).then(function() { + assertStyle(dims, + ['rgb(255, 0, 0)', 'rgb(0, 0, 255)'], + [1, 1] + ); + + return Plotly.restyle(gd, 'marker.opacity', 0.4); + }).then(function() { + assertStyle(dims, + ['rgb(255, 0, 0)', 'rgb(0, 0, 255)'], + [0.4, 0.4] + ); + + expect(gd._fullData[0].marker.opacity).toEqual(0.4); + expect(gd._fullData[1].marker.opacity).toEqual(0.4); + + return Plotly.restyle(gd, 'marker.opacity', 1); + }).then(function() { + assertStyle(dims, + ['rgb(255, 0, 0)', 'rgb(0, 0, 255)'], + [1, 1] + ); + + expect(gd._fullData[0].marker.opacity).toEqual(1); + expect(gd._fullData[1].marker.opacity).toEqual(1); + + return Plotly.restyle(gd, { + 'transforms[0].style': { + a: {marker: {color: 'green'}}, + b: {marker: {color: 'red'}} + }, + 'marker.opacity': 0.4 + }); + }).then(function() { + assertStyle(dims, + ['rgb(0, 128, 0)', 'rgb(255, 0, 0)'], + [0.4, 0.4] + ); + + done(); + }); + }); + it('Plotly.restyle should work', function(done) { var data = Lib.extendDeep([], mockData0); data[0].marker = { size: 20 }; @@ -98,10 +161,10 @@ describe('groupby', function() { expect(gd._fullData[1].marker.opacity).toEqual(1); return Plotly.restyle(gd, { - 'transforms[0].style': [ + 'transforms[0].style': [[ {target: 'a', value: {marker: {color: 'green'}}}, {target: 'b', value: {marker: {color: 'red'}}} - ], + ]], 'marker.opacity': 0.4 }); }).then(function() { @@ -401,7 +464,7 @@ describe('groupby', function() { groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], style: [ {target: 'a', value: {marker: {color: 'red'}}}, - {taret: 'b', value: {marker: {color: 'blue'}}} + {target: 'b', value: {marker: {color: 'blue'}}} ] }] }]; From 7beddad339b642a7b102c076c99156411be27214 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 12:16:22 -0500 Subject: [PATCH 3/8] Fix groupby style docs --- src/transforms/groupby.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index a5668781a79..f4299cb3c48 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -49,7 +49,7 @@ exports.attributes = { description: [ 'Sets each group style.', 'For example, with `groups` set to *[\'a\', \'b\', \'a\', \'b\']*', - 'and `style` set to *{ a: { marker: { color: \'red\' } }}', + 'and `style` set to *[{target: \'a\': { marker: { color: \'red\' } }}]', 'marker points in group *\'a\'* will be drawn in red.' ].join(' ') }, From f67198b7f777ec54baf36a6c8c391e193f3273a8 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 13:33:59 -0500 Subject: [PATCH 4/8] Move old-style groupby styles fix to cleanData --- src/plot_api/helpers.js | 38 ++++++++++++++------ src/transforms/groupby.js | 24 ++++--------- test/jasmine/tests/transform_groupby_test.js | 20 +++-------- 3 files changed, 37 insertions(+), 45 deletions(-) diff --git a/src/plot_api/helpers.js b/src/plot_api/helpers.js index 26c03943d6a..9e6baae57e7 100644 --- a/src/plot_api/helpers.js +++ b/src/plot_api/helpers.js @@ -215,7 +215,6 @@ function cleanAxRef(container, attr) { // Make a few changes to the data right away // before it gets used for anything exports.cleanData = function(data, existingData) { - // Enforce unique IDs var suids = [], // seen uids --- so we can weed out incoming repeats uids = data.concat(Array.isArray(existingData) ? existingData : []) @@ -348,18 +347,35 @@ exports.cleanData = function(data, existingData) { if(!Lib.isPlainObject(transform)) continue; - if(transform.type === 'filter') { - if(transform.filtersrc) { - transform.target = transform.filtersrc; - delete transform.filtersrc; - } + switch(transform.type) { + case 'filter': + if(transform.filtersrc) { + transform.target = transform.filtersrc; + delete transform.filtersrc; + } - if(transform.calendar) { - if(!transform.valuecalendar) { - transform.valuecalendar = transform.calendar; + if(transform.calendar) { + if(!transform.valuecalendar) { + transform.valuecalendar = transform.calendar; + } + delete transform.calendar; + } + break; + + case 'groupby': + if(transform.style && !Array.isArray(transform.style)) { + var prevStyles = transform.style; + var styleKeys = Object.keys(prevStyles); + + transform.style = []; + for(var j = 0; j < styleKeys.length; j++) { + transform.style.push({ + target: styleKeys[j], + value: prevStyles[styleKeys[j]] + }); + } } - delete transform.calendar; - } + break; } } } diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index f4299cb3c48..1291ce71fac 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -85,17 +85,12 @@ exports.supplyDefaults = function(transformIn) { var styleIn = transformIn.style; var styleOut = transformOut.style = []; - if(Array.isArray(styleIn)) { - // If styles are an array, then sanitize them against the schema: + if(styleIn) { for(var i = 0; i < styleIn.length; i++) { styleOut[i] = {}; Lib.coerce(styleIn[i], styleOut[i], exports.attributes.style, 'target'); Lib.coerce(styleIn[i], styleOut[i], exports.attributes.style, 'value'); } - } else { - // Otherwise, this is deprecated keyed-object-style styles and we - // just pass the object through as the index itself: - transformOut.style = transformIn.style; } return transformOut; @@ -156,16 +151,9 @@ function transformOne(trace, state) { var arrayAttrs = PlotSchema.findArrayAttributes(trace); var style = opts.style || []; - var styleIndex = style; - - // If the styles are an array, then translate it into an index keyed on the - // target. Otherwise just use the object provided for the sake of backward - // compatibility. - if(Array.isArray(style)) { - styleIndex = {}; - for(i = 0; i < style.length; i++) { - styleIndex[style[i].target] = style[i].value; - } + var styleLookup = {}; + for(i = 0; i < style.length; i++) { + styleLookup[style[i].target] = style[i].value; } for(i = 0; i < groupNames.length; i++) { @@ -183,9 +171,9 @@ function transformOne(trace, state) { newTrace.name = groupName; - // there's no need to coerce styleIndex[groupName] here + // there's no need to coerce styleLookup[groupName] here // as another round of supplyDefaults is done on the transformed traces - newTrace = Lib.extendDeepNoArrays(newTrace, styleIndex[groupName] || {}); + newTrace = Lib.extendDeepNoArrays(newTrace, styleLookup[groupName] || {}); } return newData; diff --git a/test/jasmine/tests/transform_groupby_test.js b/test/jasmine/tests/transform_groupby_test.js index b2c4c9f965c..6d1ed150d33 100644 --- a/test/jasmine/tests/transform_groupby_test.js +++ b/test/jasmine/tests/transform_groupby_test.js @@ -6,7 +6,6 @@ var destroyGraphDiv = require('../assets/destroy_graph_div'); var assertDims = require('../assets/assert_dims'); var assertStyle = require('../assets/assert_style'); - describe('groupby', function() { describe('one-to-many transforms:', function() { @@ -109,22 +108,11 @@ describe('groupby', function() { expect(gd._fullData[0].marker.opacity).toEqual(1); expect(gd._fullData[1].marker.opacity).toEqual(1); + }).then(done); - return Plotly.restyle(gd, { - 'transforms[0].style': { - a: {marker: {color: 'green'}}, - b: {marker: {color: 'red'}} - }, - 'marker.opacity': 0.4 - }); - }).then(function() { - assertStyle(dims, - ['rgb(0, 128, 0)', 'rgb(255, 0, 0)'], - [0.4, 0.4] - ); - - done(); - }); + // The final test for restyle updates using deprecated syntax + // is ommitted since old style syntax is *only* sanitized on + // initial plot, *not* on restyle. }); it('Plotly.restyle should work', function(done) { From 3cc16ba55ad58c726b6990ab35145bccf809648b Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 13:45:06 -0500 Subject: [PATCH 5/8] Switch groupby style.target to string --- src/transforms/groupby.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 1291ce71fac..e27d6015094 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -36,7 +36,7 @@ exports.attributes = { }, style: { target: { - valType: 'any', + valType: 'string', role: 'info', description: [ 'The group value which receives these styles.' From 1543892abe9ae6b8c3474194436f07f1502ea426 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 14:04:26 -0500 Subject: [PATCH 6/8] Fix multitransform test --- test/jasmine/tests/transform_multi_test.js | 40 +++++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/test/jasmine/tests/transform_multi_test.js b/test/jasmine/tests/transform_multi_test.js index 6930effbd8e..f08dcbfb558 100644 --- a/test/jasmine/tests/transform_multi_test.js +++ b/test/jasmine/tests/transform_multi_test.js @@ -225,7 +225,13 @@ describe('multiple transforms:', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [{ + target: 'a', + value: {marker: {color: 'red'}}, + }, { + target: 'b', + value: {marker: {color: 'blue'}} + }] }, { type: 'filter', operation: '>' @@ -239,7 +245,13 @@ describe('multiple transforms:', function() { transforms: [{ type: 'groupby', groups: ['b', 'a', 'b', 'b', 'b', 'a', 'a'], - style: { a: {marker: {color: 'green'}}, b: {marker: {color: 'black'}} } + style: [{ + target: 'a', + value: {marker: {color: 'green'}} + }, { + target: 'b', + value: {marker: {color: 'black'}} + }] }, { type: 'filter', operation: '<', @@ -329,7 +341,13 @@ describe('multiple transforms:', function() { expect(gd._fullData[1].marker.opacity).toEqual(1); return Plotly.restyle(gd, { - 'transforms[0].style': { a: {marker: {color: 'green'}}, b: {marker: {color: 'red'}} }, + 'transforms[0].style': [[{ + target: 'a', + value: {marker: {color: 'green'}} + }, { + target: 'b', + value: {marker: {color: 'red'}} + }]], 'marker.opacity': 0.4 }); }).then(function() { @@ -437,7 +455,13 @@ describe('multiple traces with transforms:', function() { transforms: [{ type: 'groupby', groups: ['a', 'a', 'b', 'a', 'b', 'b', 'a'], - style: { a: {marker: {color: 'red'}}, b: {marker: {color: 'blue'}} } + style: [{ + target: 'a', + value: {marker: {color: 'red'}}, + }, { + target: 'b', + value: {marker: {color: 'blue'}} + }] }, { type: 'filter', operation: '>' @@ -508,7 +532,13 @@ describe('multiple traces with transforms:', function() { }); return Plotly.restyle(gd, { - 'transforms[0].style': { a: {marker: {color: 'green'}}, b: {marker: {color: 'red'}} }, + 'transforms[0].style': [[{ + target: 'a', + value: {marker: {color: 'green'}}, + }, { + target: 'b', + value: {marker: {color: 'red'}} + }]], 'marker.opacity': [0.4, 0.6] }); }).then(function() { From 8df0096551645901c9c77b876584d44cf39e38eb Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 14:28:42 -0500 Subject: [PATCH 7/8] Add isLinkedToArray to groupby style attrs --- src/transforms/groupby.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index e27d6015094..6e0baae0278 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -35,6 +35,7 @@ exports.attributes = { ].join(' ') }, style: { + _isLinkedToArray: 'style', target: { valType: 'string', role: 'info', From 49e24ad1ec4c86f0c2c134b0d484bf0d58d1477a Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Fri, 10 Mar 2017 14:46:28 -0500 Subject: [PATCH 8/8] Fix typo in groupby style doc string --- src/transforms/groupby.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transforms/groupby.js b/src/transforms/groupby.js index 6e0baae0278..dba79dfc878 100644 --- a/src/transforms/groupby.js +++ b/src/transforms/groupby.js @@ -50,7 +50,7 @@ exports.attributes = { description: [ 'Sets each group style.', 'For example, with `groups` set to *[\'a\', \'b\', \'a\', \'b\']*', - 'and `style` set to *[{target: \'a\': { marker: { color: \'red\' } }}]', + 'and `style` set to *[{target: \'a\', value: { marker: { color: \'red\' } }}]', 'marker points in group *\'a\'* will be drawn in red.' ].join(' ') },