Allow data dependencies in data binding

Allow data dependencies in data binding

Binding elements can indicate they depend on the values of 
other model fields. An include test shows how this works.

In viewlets you can extend the bindings section of its configuration
to include 'depends'. A list of model attribute key names on which
this fields depends.

bindings: {
  full: {
     depends: ['first', 'last']
		}
}

In this case we might indicate the computed value 'full' (name)
depends on first and last name stored in the model. Full Name
would be an attribute with a getter but doesn't fire change events
itself to trigger model updates. The attribute definition for 'full'
might be as follows:

 full: {
   getter: function() {return this.get('first') +
              ' '  + this.get('last');}
 }

https://code.launchpad.net/~bcsaller/juju-gui/databinding-deps/+merge/172661

(do not edit description out of merge proposal)

[bcsaller, 2013-07-02 20:15:46]

Please take a look.

[jeff.pihach, 2013-07-02 20:39:10]

LGTM - good work! I'd like to see one extra assertion as mentioned in the
comments.

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

https://codereview.appspot.com/10760046/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:69: // XXX: Too many copies, but we don't want to
modify the list
I'm not really sure a filter function should have side effects, like pushing to
result) so this comment isn't really necessary.

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

https://codereview.appspot.com/10760046/diff/1/test/test_databinding.js#newco...
test/test_databinding.js:281: model.set('last', 'Lannister');
Should probably add in a test for setAttrs() JIC

https://codereview.appspot.com/10760046/diff/1/test/test_databinding.js#newco...
test/test_databinding.js:284: 
Extra spacing here probably isn't intentional

[benjamin.saller, 2013-07-02 21:26:49]

Thanks for the review, correctly locally as they are minors.

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

https://codereview.appspot.com/10760046/diff/1/test/test_databinding.js#newco...
test/test_databinding.js:281: model.set('last', 'Lannister');
On 2013/07/02 20:39:10, jeff.pihach wrote:
> Should probably add in a test for setAttrs() JIC

Changed one of the sets, but I think wrt to set/setAttrs one calls the other
under the hood.

[gary.poster, 2013-07-02 22:13:54]

LGTM with trivials.  Thank you!

Gary

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

https://codereview.appspot.com/10760046/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:69: // XXX: Too many copies, but we don't want to
modify the list
On 2013/07/02 20:39:10, jeff.pihach wrote:
> I'm not really sure a filter function should have side effects, like pushing
to
> result) so this comment isn't really necessary. 

The filter function returns a new array, on which Ben calls forEach.  I'm
assuming the temporary new array is what Ben is talking about.  I really doubt
the scale/usage is worth worrying about.  If it is, you could simply change the
filter into a conditional within a forEach block on the original "bindings"
array.  However, I think you can just remove this XXX comment.

https://codereview.appspot.com/10760046/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:409: // binding target in the DOM)
LOL, that's deep, man. :-)  I wonder if that would ever happen.  Nice
cleanliness, either way.

https://codereview.appspot.com/10760046/diff/1/test/test_databinding.js
File test/test_databinding.js (left):

https://codereview.appspot.com/10760046/diff/1/test/test_databinding.js#oldco...
test/test_databinding.js:80: it('inherits unctions from object attributes to its
properties',
unction
Noun
The action of anointing someone with oil or ointment as a religious rite or as a
symbol of investiture as a monarch.
The oil or ointment so used.

I think it was cooler before you fixed the typo. :-)

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

https://codereview.appspot.com/10760046/diff/1/test/test_databinding.js#newco...
test/test_databinding.js:283: .get('value'), 'Sansa Lannister');
poor Sansa.  She is kind of annoying, but...still.

[benjamin.saller, 2013-07-02 22:18:45]

Thanks for the reviews

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

https://codereview.appspot.com/10760046/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:69: // XXX: Too many copies, but we don't want to
modify the list
On 2013/07/02 22:13:55, gary.poster wrote:
> On 2013/07/02 20:39:10, jeff.pihach wrote:
> > I'm not really sure a filter function should have side effects, like pushing
> to
> > result) so this comment isn't really necessary. 
> 
> The filter function returns a new array, on which Ben calls forEach.  I'm
> assuming the temporary new array is what Ben is talking about.  I really doubt
> the scale/usage is worth worrying about.  If it is, you could simply change
the
> filter into a conditional within a forEach block on the original "bindings"
> array.  However, I think you can just remove this XXX comment.

Yes that was the copy. I removed the comment.

https://codereview.appspot.com/10760046/diff/1/app/views/databinding.js#newco...
app/views/databinding.js:409: // binding target in the DOM)
On 2013/07/02 22:13:55, gary.poster wrote:
> LOL, that's deep, man. :-)  I wonder if that would ever happen.  Nice
> cleanliness, either way.

The test actually shows this. First name is bound and full name but not last
name. We generate a binding for 'last' so we can trigger full name to update but
it has no target.

[bcsaller, 2013-07-02 22:24:37]

*** Submitted:

Allow data dependencies in data binding

Binding elements can indicate they depend on the values of 
other model fields. An include test shows how this works.

In viewlets you can extend the bindings section of its configuration
to include 'depends'. A list of model attribute key names on which
this fields depends.

bindings: {
  full: {
     depends: ['first', 'last']
		}
}

In this case we might indicate the computed value 'full' (name)
depends on first and last name stored in the model. Full Name
would be an attribute with a getter but doesn't fire change events
itself to trigger model updates. The attribute definition for 'full'
might be as follows:

 full: {
   getter: function() {return this.get('first') +
              ' '  + this.get('last');}
 }

R=jeff.pihach, benjamin.saller, gary.poster
CC=
https://codereview.appspot.com/10760046