:-) 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#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#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.
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-descripti on.handlebars charm-descripti on.handlebars (right):
File app/templates/
https:/ /codereview. appspot. com/6819098/ diff/6001/ app/templates/ charm-descripti on.handlebars# newcode2 charm-descripti on.handlebars: 2: <div charm-nav- back">< i class=" icon-chevron- left">< /i> Back</div>
app/templates/
class="
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-descripti on.handlebars# newcode10 charm-descripti on.handlebars: 10: <div charm-panel- configure- buttons" >
app/templates/
class="
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-descripti on.handlebars# newcode23 charm-descripti on.handlebars: 23: {{#if owner}}
app/templates/
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-descripti on.handlebars# newcode60 charm-descripti on.handlebars: 60: <h4><i icon-chevron- up"></i> Change Log</h4>
app/templates/
class="
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 charm-pre- configuration. handlebars (right):
File app/templates/
https:/ /codereview. appspot. com/6819098/ diff/6001/ app/templates/ charm-pre- configuration. handlebars# newcode43 charm-pre- configuration. handlebars: 43: <div config- file-upload control-group">
app/templates/
class="
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 stylesheet. less (right):
File lib/views/
https:/ /codereview. appspot. com/6819098/ diff/6001/ lib/views/ stylesheet. less#newcode785 stylesheet. less:785: } stackoverflow. com/questions/ 4053220/ css-scrollbar- width). I
lib/views/
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://
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 charm_panel. js (right):
File test/test_
https:/ /codereview. appspot. com/6819098/ diff/6001/ test/test_ charm_panel. js#newcode253 charm_panel. js:253: section_container = 'div.charm- section: last-child' );
test/test_
html.one(
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/