Hi Benji, thanks for the improvement. I love the multiple "var" declarations... and Douglas Crockford agree with us. :O)
The feature seems to be broken. I have a comment for that. []s, Thiago. 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#newcode58 app/views/service.js:58: this._modifyUnits(parseInt(fieldValue, 10)); I've found a YUI utility for it. http://yuilibrary.com/yui/docs/api/classes/Number.html "parse" https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode76 app/views/service.js:76: var field = this._getInputBox(); This feature is not working. I cannot change the field value. If you change this method to... handleKeyEvent: function(ev) { if (ev.keyCode !== ESC && ev.keyCode !== ENTER) { return; } var field = this._getInputBox(); ev.halt(true); this._handleKeyPress(ev.keyCode, field.get('value')); }, ... it starts working again. I think it is something related to the "field.get('value')" method call. https://codereview.appspot.com/6845065/diff/1/app/views/service.js#newcode91 app/views/service.js:91: env = this.get('env'), env never used? 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#newcode26 docs/style-guide.rst:26: var a = 1; A big +1 for it! Thanks! 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#newcode138 test/test_app.js:138: container.hide(); In other test files we simply remove the elements from the DOM. Probably you could do the same. https://codereview.appspot.com/6845065/diff/1/test/test_app.js#newcode199 test/test_app.js:199: container = Y.Node.create('<div/>') Why? We already remove it from the dom after every test. https://codereview.appspot.com/6845065/diff/1/test/test_application_notifications.js File test/test_application_notifications.js (right): https://codereview.appspot.com/6845065/diff/1/test/test_application_notifications.js#newcode42 test/test_application_notifications.js:42: applicationContainer = Y.Node.create('<div/>').hide(); Why? We already remove it from the dom after every test. https://codereview.appspot.com/6845065/diff/1/test/test_charm_collection_view.js File test/test_charm_collection_view.js (right): https://codereview.appspot.com/6845065/diff/1/test/test_charm_collection_view.js#newcode108 test/test_charm_collection_view.js:108: var container = Y.Node.create('<div/>').hide(); We already destroy it in this method. I don't think we need to hide it. https://codereview.appspot.com/6845065/diff/1/test/test_charm_view.js File test/test_charm_view.js (right): https://codereview.appspot.com/6845065/diff/1/test/test_charm_view.js#newcode57 test/test_charm_view.js:57: container = Y.Node.create('<div/>').hide(); ditto https://codereview.appspot.com/6845065/diff/1/test/test_environment_view.js File test/test_environment_view.js (right): https://codereview.appspot.com/6845065/diff/1/test/test_environment_view.js#newcode106 test/test_environment_view.js:106: container = Y.Node.create('<div/>') We already destroy it. 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#newcode3 test/test_manageUnitsMixin.js:3: (function() { Why do you wrap everything inside this function? "describe" already does that. https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#newcode21 test/test_manageUnitsMixin.js:21: done(); You dont need to call it. In fact, you should avoid the use of this magic "done" method. https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#newcode25 test/test_manageUnitsMixin.js:25: container = Y.Node.create('<div id=\"test-container\"/>').hide(); Avoid "done". This is not a async execution. https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#newcode31 test/test_manageUnitsMixin.js:31: container.remove(true); If you keep "done", you risk of having a call to an "it" method before the previous "it" execution calls the "afterEach" method. https://codereview.appspot.com/6845065/diff/1/test/test_manageUnitsMixin.js#newcode36 test/test_manageUnitsMixin.js:36: var fauxField = {}; I like the var declaration! Thnaks! https://codereview.appspot.com/6845065/diff/1/test/test_service_view.js File test/test_service_view.js (right): https://codereview.appspot.com/6845065/diff/1/test/test_service_view.js#newcode26 test/test_service_view.js:26: container = Y.Node.create('<div/>').hide(); ditto 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
