yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

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.