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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
