From 6d2f65573d46cc110fd607c5e276dc91e44dbc98 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Mon, 21 Nov 2016 12:52:18 -0500 Subject: [PATCH 1/5] Add check for failed binding comparison --- src/plots/command.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/plots/command.js b/src/plots/command.js index cb3b42fb8d3..3dc54f45ecc 100644 --- a/src/plots/command.js +++ b/src/plots/command.js @@ -145,11 +145,12 @@ exports.manageCommandObserver = function(gd, container, commandList, onchange) { * 3. the same property must be affected by all commands */ exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue) { + var i; var n = commandList.length; var refBinding; - for(var i = 0; i < n; i++) { + for(i = 0; i < n; i++) { var binding; var command = commandList[i]; var method = command.method; @@ -207,6 +208,8 @@ exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue) } } + if(i === n) return false; + return refBinding; }; From 6dc01d6569846cd4c039663f857db3383f672901 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Mon, 21 Nov 2016 13:06:49 -0500 Subject: [PATCH 2/5] Make 'is simple binding' false for non-length-one arrays --- src/plots/command.js | 8 +++++--- test/jasmine/tests/command_test.js | 17 +++++++++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/plots/command.js b/src/plots/command.js index 3dc54f45ecc..b1d86fa23f8 100644 --- a/src/plots/command.js +++ b/src/plots/command.js @@ -201,15 +201,17 @@ exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue) binding = bindings[0]; var value = binding.value; if(Array.isArray(value)) { - value = value[0]; + if(value.length === 1) { + value = value[0]; + } else { + return false; + } } if(bindingsByValue) { bindingsByValue[value] = i; } } - if(i === n) return false; - return refBinding; }; diff --git a/test/jasmine/tests/command_test.js b/test/jasmine/tests/command_test.js index b88e2613c5a..9080b6faf97 100644 --- a/test/jasmine/tests/command_test.js +++ b/test/jasmine/tests/command_test.js @@ -155,12 +155,17 @@ describe('Plots.hasSimpleAPICommandBindings', function() { args: [{'marker.color': 20}, [2, 1]] }]); - expect(isSimple).toEqual({ + // See https://github.com/plotly/plotly.js/issues/1169 for an example of where + // this logic was a little too sophisticated. It's better to bail out and omit + // functionality than to get it wrong. + expect(isSimple).toEqual(false); + + /* expect(isSimple).toEqual({ type: 'data', prop: 'marker.color', traces: [ 1, 2 ], value: [ 10, 10 ] - }); + });*/ }); }); @@ -508,6 +513,14 @@ describe('component bindings', function() { }).catch(fail).then(done); }); + it('udpates bound components when the value changes', function(done) { + expect(gd.layout.sliders[0].active).toBe(0); + + Plotly.restyle(gd, 'marker.color', 'ecru').then(function() { + expect(gd.layout.sliders[0].active).toBe(0); + }).catch(fail).then(done); + }); + it('udpates bound components when the computed value changes', function(done) { expect(gd.layout.sliders[0].active).toBe(0); From 242f3b47362f2e7b8c82a80f6156f21e04c3644a Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Mon, 21 Nov 2016 13:18:18 -0500 Subject: [PATCH 3/5] Add test for non-present command arg value --- test/jasmine/tests/command_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/command_test.js b/test/jasmine/tests/command_test.js index 9080b6faf97..f052b683cfb 100644 --- a/test/jasmine/tests/command_test.js +++ b/test/jasmine/tests/command_test.js @@ -513,7 +513,7 @@ describe('component bindings', function() { }).catch(fail).then(done); }); - it('udpates bound components when the value changes', function(done) { + it('does not update the component if the value is not present', function(done) { expect(gd.layout.sliders[0].active).toBe(0); Plotly.restyle(gd, 'marker.color', 'ecru').then(function() { From 218744d25465a956a10c36841aad3ff567698fbc Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Mon, 21 Nov 2016 13:20:21 -0500 Subject: [PATCH 4/5] Fix command api test that doesn't actually test anything --- test/jasmine/tests/command_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/command_test.js b/test/jasmine/tests/command_test.js index f052b683cfb..01725c22e39 100644 --- a/test/jasmine/tests/command_test.js +++ b/test/jasmine/tests/command_test.js @@ -516,7 +516,7 @@ describe('component bindings', function() { it('does not update the component if the value is not present', function(done) { expect(gd.layout.sliders[0].active).toBe(0); - Plotly.restyle(gd, 'marker.color', 'ecru').then(function() { + Plotly.restyle(gd, 'marker.color', 'black').then(function() { expect(gd.layout.sliders[0].active).toBe(0); }).catch(fail).then(done); }); From 62c95f810f9f113308914b7a37b91b50583d3854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Mon, 21 Nov 2016 14:08:39 -0500 Subject: [PATCH 5/5] test: add jasmine test for #1169 --- test/jasmine/tests/updatemenus_test.js | 46 ++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test/jasmine/tests/updatemenus_test.js b/test/jasmine/tests/updatemenus_test.js index b34558d3491..c415b0ccbd3 100644 --- a/test/jasmine/tests/updatemenus_test.js +++ b/test/jasmine/tests/updatemenus_test.js @@ -462,6 +462,52 @@ describe('update menus interactions', function() { }); }); + it('should update correctly on failed binding comparisons', function(done) { + + // See https://github.com/plotly/plotly.js/issues/1169 + // for more info. + + var data = [{ + y: [1, 2, 3], + visible: true + }, { + y: [2, 3, 1], + visible: false + }, { + y: [3, 1, 2], + visible: false + }]; + + var layout = { + updatemenus: [{ + buttons: [{ + label: 'a', + method: 'restyle', + args: ['visible', [true, false, false]] + }, { + label: 'b', + method: 'restyle', + args: ['visible', [false, true, false]] + }, { + label: 'c', + method: 'restyle', + args: ['visible', [false, false, true]] + }] + }] + }; + + Plotly.newPlot(gd, data, layout).then(function() { + return click(selectHeader(0)); + }) + .then(function() { + return click(selectButton(1)); + }) + .then(function() { + assertActive(gd, [1]); + }) + .then(done); + }); + it('should change color on mouse over', function(done) { var INDEX_0 = 2, INDEX_1 = gd.layout.updatemenus[1].active;