yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Upon grabbing your branch for testing I see there are conflicts with
trunk which include the issue of -right vs -up.

The branch looks good and behaved nicely.

Of course, wait for a Real Review.


https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode8
app/views/charm-search.js:8: subscriptions = [],
A comment here describing the use and purpose of 'subscriptions' would
be really helpful. I figured out but only when I got to the end of this
file.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode15
app/views/charm-search.js:15: icon = ev.currentTarget.one('i');
Is this line really repeated?

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode18
app/views/charm-search.js:18: icon.replaceClass('icon-chevron-right',
'icon-chevron-down');
-right should be -up to match the visual design. If you think the design
is wrong then the charm panel section headers need to change too.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode438
app/views/charm-search.js:438: container =
Y.Node.create('<div></div>').setAttribute(
Any tag created by Y.Node.create can be self-closing and it does the
right thing.  So

Y.Node.create('<div />')

works and is more readable, though you may argue misleading.  I see we
do both.  Just mentioning it.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode439
app/views/charm-search.js:439: 'id', 'juju-search-charm-panel'),
Indent more

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode518
app/views/charm-search.js:518: headerSpan.setStyle('borderLeftColor',
'lightgray');
Why isn't this styling in CSS?

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode534
app/views/charm-search.js:534: if (isPopupVisible) {
These names with 'Popup' are historical and now inaccurate.  Other
places you refer to the panel (calculatePanelPosition).  The use of both
is confusing.

https://codereview.appspot.com/6775058/diff/18001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/6775058/diff/18001/app/views/environment.js#newcode1236
app/views/environment.js:1236: // "afterPageSizeRecalculation" event at
the end of this function.
Nice comment.

https://codereview.appspot.com/6775058/

-- 
https://code.launchpad.net/~gary/juju-gui/charmpanel/+merge/131605
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~gary/juju-gui/charmpanel into lp:juju-gui.