Code review comment for lp:~frankban/juju-gui/bug-1075679-charm-config-panel

Revision history for this message
Brad Crittenden (bac) wrote :

Nice changes Francesco.

I think the config file button still needs some attention. I find it
odd that it isn't part of the collapsible div and remains visible when
Service Settings is collapsed. Did you discuss that UX with Matt or
Jovan? Of course I defer to them but think it currently looks funny.

Also, when a file is chosen, Service Settings collapses but the chevron
still points down. Further you can click on it and the chevron changes
but nothing else happens. I guess it'd be nice to collapse, set the
chevron to up, and lock it in place.

https://codereview.appspot.com/6827066/diff/1/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/6827066/diff/1/app/views/charm-panel.js#newcode601
app/views/charm-panel.js:601: */
Nice, thanks.

https://codereview.appspot.com/6827066/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/6827066/diff/1/lib/views/stylesheet.less#newcode43
lib/views/stylesheet.less:43:
Thanks! Maybe 'set-' instead of 'create-' for both?

https://codereview.appspot.com/6827066/diff/1/lib/views/stylesheet.less#newcode987
lib/views/stylesheet.less:987: padding-left: 5px;
The vis design shows this lined up with the other elements and the charm
icon up top. I think the padding-left should be 11px.

Good to see you and Matt discussing it.

And as I mentioned in IRC we should decide on one value and apply it to
all charm panels, abstracting it into a LESS variable.

https://codereview.appspot.com/6827066/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/6827066/diff/1/undocumented#oldcode82
undocumented:82: app/views/charm-panel.js render
yay

https://codereview.appspot.com/6827066/

« Back to merge proposal