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

Revision history for this message
Benjamin Saller (bcsaller) 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.

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.

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

« Back to merge proposal