Code review comment for lp://staging/~gary/juju-gui/databindingChangedValues

Revision history for this message
Richard Harding (rharding) wrote :

LGTM with notes below. Will QA in a sec.

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode62
app/views/databinding.js:62: return (normalizedValue === currentValue);
should we refactor this out into any _stringEq or something since it's
duped in the three eq methods?

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode688
app/views/databinding.js:688: if (nodeHandler.eq(e.target,
binding.get(model))) {
will this blow up if no binding was found matching key?

https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode875
app/views/databinding.js:875: 'event-valuechange']
is this required in this module? It's used in the inspector I believe,
but not here.

https://codereview.appspot.com/13385045/diff/1/test/test_databinding.js
File test/test_databinding.js (right):

https://codereview.appspot.com/13385045/diff/1/test/test_databinding.js#newcode26
test/test_databinding.js:26: 'model', 'model-list',
'node-event-simulate'];
this needs the event-valueChange since you are binding with that event
below.

https://codereview.appspot.com/13385045/

« Back to merge proposal