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

On 2013/09/11 01:37:51, benjamin.saller wrote:
> LGTM, minor suggestions, take it or leave it, thanks for the cleanups

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

https://codereview.appspot.com/13265045/diff/1/app/views/databinding.js#newcode668
> app/views/databinding.js:668: @return {Binding} Binding reference.
> This is a case, but not the only one, a single model key can have more
than one
> node bound and thus more than one binding. Depending on your use case
this maybe
> ok, but I suspect you'd always want to return an array of all bindings
for a
> given key.

> The other option that works with the current code is to also pass a
node target,
> and say get the binding for (key) where binding.target === target.
This should
> always be one.

Ah! Great point, thank you. I'll take the second route. That's what
this use case needs.

https://codereview.appspot.com/13265045/diff/1/app/views/databinding.js#newcode711
> app/views/databinding.js:711: if (nodeHandler.eq(node,
binding.get(model))) {
> This reads nicely now, thank you. See the notes about getBinding
though

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

https://codereview.appspot.com/13265045/diff/1/test/test_databinding.js#newcode35
> test/test_databinding.js:35: };
> nice cleanup, I'd be tempted to add 'options' to the arguments and mix
it in
> object create to allow add/overwrite viewlet code but you contain a
superset of
> what the tests need so no worries.

Yeah, I like that idea, but I'll leave that to The Future. :-)

Thank you!

https://codereview.appspot.com/13265045/