yellow team mailing list archive

[Matthew Scott <matthew.scott@xxxxxxxxxxxxx>]

This all looks pretty good to me, with a few little
minors/clarifications and Thiago's comments.  Eager to see it plugged
in, for sure.



https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode44
app/assets/javascripts/d3-components.js:44: * Component collections
modules implementing various portions
Minor - should be 'collects', I think?  Was just confusing.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode52
app/assets/javascripts/d3-components.js:52: *    - Providing suggestions
around updating the interactive portions
Minor - 'updating'

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode70
app/assets/javascripts/d3-components.js:70: *
Minor - Docs don't match method signature.

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode97
app/assets/javascripts/d3-components.js:97: this.bind(module.name);
Curious about the necessity for not doing Y.clone(module.events); here.
Clarity?

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode124
app/assets/javascripts/d3-components.js:124: phase = 'on',
phase is set and carefully handled, but not used.  Is this a future
implementation thing?

https://codereview.appspot.com/6828048/diff/1/app/assets/javascripts/d3-components.js#newcode189
app/assets/javascripts/d3-components.js:189: //       where object
includes phase (before, on, after)
 From above, assuming this is the future use of phase?

https://codereview.appspot.com/6828048/

-- 
https://code.launchpad.net/~bcsaller/juju-gui/component-framework/+merge/133391
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~bcsaller/juju-gui/component-framework into lp:juju-gui.