yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Thank you for the review Brad.  I've made all requested changes.  And
intend to land this, per the new rules.

Gary


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 = [],
On 2012/10/30 11:12:44, bac wrote:
> 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.

Done.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode15
app/views/charm-search.js:15: icon = ev.currentTarget.one('i');
On 2012/10/30 11:12:44, bac wrote:
> Is this line really repeated?

Heh, yes.  The duplication was in trunk (see lines 14 and 15 at left)
but I did copy it blindly.  Fixed, thank you.

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');
On 2012/10/30 11:12:44, bac wrote:
> -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.

I believe I've changed everything as desired.

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(
On 2012/10/30 11:12:44, bac wrote:
> 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.

Yeah, I know.  I felt mixed about the short form.  I ended up using 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'),
On 2012/10/30 11:12:44, bac wrote:
> Indent more

Done.

https://codereview.appspot.com/6775058/diff/18001/app/views/charm-search.js#newcode518
app/views/charm-search.js:518: headerSpan.setStyle('borderLeftColor',
'lightgray');
On 2012/10/30 11:12:44, bac wrote:
> Why isn't this styling in CSS?

We talked about the explanation, but I moved it to a class.

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

Yeah...this comment led to a lot of changes :-)  I corrected Panel ->
Popup and I also corrected many instances of search -> panel, because
that is also wrong.

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.