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

On 2013/09/07 06:33:20, 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?

We discussed this and the other points via email (I thought Rietveld
would get the comments). Done.

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?

In sum, this was intentional, because it should never blow up if
everything is working properly before this point in the code. I changed
the code to make this more explicit.

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.

On further discussion, I pointed out that the valueChange subscription
is in databinding, so Rick agreed with me that what I did was fine.

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.

As above.

Thank you, Rick!

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