Fix data binding check #7
Conversation
* Added lookup table to search commands by value. Fixes plotly#1169
* Test that API command binding accepts data arrays.
|
This looks great! Unless I'm wrong, it should be able to avoid pathological cases gracefully. The relevant cases seem to be:
|
This PR wouldn't do a deep comparison. Shall I check here whether |
|
As long as there's no deep comparison, I think it should be fine, but yeah, probably best to stop this at the hasSimpleBindings level. Thanks for cleaning this up! I failed to appreciate the importance of visibility toggles like this. Is this PR intended for the main plotly.js repo? (It'd definitely be a welcome fix!) |
|
(Hope the code wasn't too difficult to work though! 😄 It was a simple concept that ended up being moderately complicated to actually implement!) |
|
|
||
| if(obj.hasOwnProperty(binding.prop)) { | ||
| if(obj[binding.prop] !== value) { | ||
| if(!thisCache.hasOwnProperty(binding.prop)) { |
|
|
||
| for(commandIndex in binding.valueByBindings) { | ||
| var commandValue = binding.valueByBindings[commandIndex]; | ||
| if(commandValue.length !== value.length) continue; |
Yes, if you're happy with this solution and the signature change in I guess since plotly#1176 fixes plotly#1169 , there is no rush to merge this PR. I'll talk to you on Slack tomorrow. |
| @@ -135,6 +139,39 @@ exports.manageCommandObserver = function(gd, container, commandList, onchange) { | |||
| return ret; | |||
| }; | |||
|
|
|||
| function getCommandIndex(binding, value) { | |||
There was a problem hiding this comment.
rename binding to observer
| @@ -135,6 +139,39 @@ exports.manageCommandObserver = function(gd, container, commandList, onchange) { | |||
| return ret; | |||
@rreusser @etpinard
This fix changes the signature of
exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue, valueByBindings)in a backwards-compatible way (I don't really like the new signature; if compatibility isn't an issue here, I'd passbindinginstead ofbindingsByValueandvalueByBindings).This change in signature was introduced so that I could search for bindings when the associated value is an array.