yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

The code is easy to follow and everything looks very readable Benji,
thank you!

I have some comments below, especially the problem of changing the unit
count using the input box. Otherwise this branch looks good and the
tests pass.



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

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode48
app/views/service.js:48: * @param {Number} keyCode The key that was
pressed.
The parameter fieldValue should be documented here.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode56
app/views/service.js:56: var numberRegex = /^\d+$/;
Thanks for defining the regex so that humans can understand what's going
on.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode77
app/views/service.js:77: ev.halt(this._handleKeyPress(ev.keyCode,
field.get('value')));
While trying this branch, I was not able to change the unit count using
the input field. It seems that this line always stops the event
propagation, and that the boolean returned by _handleKeyPress is only
used as the `immediate` arg of ev.halt. However, didn't investigated,
so, maybe I am wrong.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode92
app/views/service.js:92: db = this.get('db');
db does not seem used by this method.

https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode111
app/views/service.js:111: * @param {Function} getUnits A function that
returns all the units for a
These callable parameters are not passed to the method: they seem to be
still called as env methods.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst
File docs/style-guide.rst (right):

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode18
docs/style-guide.rst:18: Variable declaration
+1 Let's see what the other guys of the squad think about this.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode50
docs/style-guide.rst:50: Naming
Cool.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode62
docs/style-guide.rst:62: - object literals have their opening brace on
the same line as the
Looks good to me.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode63
docs/style-guide.rst:63: equals sign (e.g., "var foo = {"
Missing closing bracket.

https://codereview.appspot.com/6845065/diff/1/docs/style-guide.rst#newcode87
docs/style-guide.rst:87: int_bad: 'nope!'},
This example seems to contradict the rules above.
There is multiple var definition (separated by commas), and the old
object literal style.

https://codereview.appspot.com/6845065/diff/1/test/test_app.js
File test/test_app.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode135
test/test_app.js:135: container = Y.Node.create('<div/>')
So much better!

https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode138
test/test_app.js:138: container.hide();
Isn't the container already hidden by the line above?

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js
File test/test_manageUnitsMixin.js (right):

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#newcode20
test/test_manageUnitsMixin.js:20: after(function(done)  {
Is this required by the testing framework?

https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#newcode111
test/test_manageUnitsMixin.js:111: it('should reset the unit count a
non-number is entered', function() {
Missing 'when' in the test description.

https://codereview.appspot.com/6845065/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/6845065/diff/1/undocumented#oldcode154
undocumented:154: app/views/service.js _modifyUnits
Thank you from the future!

https://codereview.appspot.com/6845065/

-- 
https://code.launchpad.net/~benji/juju-gui/bug-1074336/+merge/135261
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~benji/juju-gui/bug-1074336 into lp:juju-gui.