yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Thanks for the review Gary.
Already merged trunk and resolved some conflicts encountered.
Now I will add fixes per UX review and land this branch.


https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars
File app/templates/charm-description.handlebars (right):

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode2
app/templates/charm-description.handlebars:2: <div
class="charm-nav-back"><i class="icon-chevron-left"></i> Back</div>
On 2012/11/07 13:58:59, gary.poster wrote:
> (As we discussed, we have a separate card for converting these to the
desired
> assets.)

Cool.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode10
app/templates/charm-description.handlebars:10: <div
class="charm-panel-configure-buttons">
On 2012/11/07 13:58:59, gary.poster wrote:
> yay, we actually reused a class name in the charm panel!  will wonders
never
> cease? :-) thank you for being thoughtful with this

:-) I have to fix other things per UX review, however this seems to be
fine. \o/

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode23
app/templates/charm-description.handlebars:23: {{#if owner}}
On 2012/11/07 13:58:59, gary.poster wrote:
> I think I have a similar block that instead  draws "[charmers]" for
the owner
> name.  I think I prefer your solution.  I don't think we have a
specification
> for this.  If we do, go with that; otherwise, make your own choice,
and I
> suggest sticking with what you have here.

Done.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-description.handlebars#newcode60
app/templates/charm-description.handlebars:60: <h4><i
class="icon-chevron-up"></i> Change Log</h4>
On 2012/11/07 13:58:59, gary.poster wrote:
> I had intentionally diverged from the "Change Log" text because I
thought a
> single change was not a log.  This is what was specified though, you
are right.
> +1 on this change.

Thanks, and I agree with you that one change is not a log.

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-configuration.handlebars
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/6819098/diff/6001/app/templates/charm-pre-configuration.handlebars#newcode43
app/templates/charm-pre-configuration.handlebars:43: <div
class="config-file-upload control-group">
On 2012/11/07 13:58:59, gary.poster wrote:
> Moving this into the service settings was a conscious decision by the
devs who
> did it, I think, even though the visual design had it here.  The
configuration
> file replaces the configuration form, and IIRC makes it hide.

> In this particular case, I suggest moving it back where it was before,
in the
> "Service Settings."  OTOH, if you disagree for any reason, you have
the visual
> design on your side, so I'm happy for this change to win in that case.
:-)

> It also doesn't look quite right--it needs some left padding--but I'm
figuring
> you want to hold off on that change for another branch.

Restored the previous position, I missed the "config file hides the
form" behavior. This config panel looks ugly but hopefully will be fixed
in another branch.

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

https://codereview.appspot.com/6819098/diff/6001/lib/views/stylesheet.less#newcode785
lib/views/stylesheet.less:785: }
On 2012/11/07 13:58:59, gary.poster wrote:
> This looks good, and I love the fact that we don't have to have JS for
this.
> OTOH, though I was reconciled to not having FF and trying to get
agreement on
> that limitation, I thought we could have IE 10: after some research
that you
> probably also did, it looks like that CSS is a bad idea (it does not
allow
> changing width, and reportedly using the IE scriollbar color CSS
forces
> older-style scrollbars, per
> http://stackoverflow.com/questions/4053220/css-scrollbar-width).  I
guess let's
> make this change and see if anyone complains--in which case I suppose
we'll have
> to try the JS thing.

Agreed.

https://codereview.appspot.com/6819098/diff/6001/test/test_charm_panel.js
File test/test_charm_panel.js (right):

https://codereview.appspot.com/6819098/diff/6001/test/test_charm_panel.js#newcode253
test/test_charm_panel.js:253: section_container =
html.one('div.charm-section:last-child');
On 2012/11/07 13:58:59, gary.poster wrote:
> This might break with my branch...or might not. :-)

It didn't!

https://codereview.appspot.com/6819098/

-- 
https://code.launchpad.net/~frankban/juju-gui/bug-1074297-charm-panel-description/+merge/133097
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/juju-gui/bug-1074297-charm-panel-description into lp:juju-gui.