yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

https://codereview.appspot.com/6724059/diff/1/app/templates/service-header.partial
File app/templates/service-header.partial (right):

https://codereview.appspot.com/6724059/diff/1/app/templates/service-header.partial#newcode15
app/templates/service-header.partial:15: <a
href="{{href}}">{{title}}</a>
On 2012/10/18 21:19:07, gary.poster wrote:
> These are supposed to be orange if the tab is active.

Hmmm... You know I am colour-blind and the specification shows the same
colour code. I will ask goodspud.

https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqS19SYWQ2MzU3cFU/edit

https://codereview.appspot.com/6724059/diff/1/app/templates/service-header.partial#newcode17
app/templates/service-header.partial:17: <div {{#if
active}}class="active"{{/if}}>
On 2012/10/18 21:19:07, gary.poster wrote:
> I'd prefer not to have this div if we could help it.  Not a huge deal,
but it
> seems to me that we could simply have colored bottom borders on the
".menu-items
> .inline.item"s to do this.

I use the image assets as required, so I need this div in order to set
the background image from the asset.

https://codereview.appspot.com/6724059/diff/1/app/templates/service.handlebars
File app/templates/service.handlebars (left):

https://codereview.appspot.com/6724059/diff/1/app/templates/service.handlebars#oldcode11
app/templates/service.handlebars:11: <div>
On 2012/10/18 21:19:07, gary.poster wrote:
> Thanks for cleaning up the tabs.

ok

https://codereview.appspot.com/6724059/diff/1/app/templates/service.handlebars
File app/templates/service.handlebars (right):

https://codereview.appspot.com/6724059/diff/1/app/templates/service.handlebars#newcode16
app/templates/service.handlebars:16: </a>
On 2012/10/18 21:19:07, gary.poster wrote:
> This duplicates code that you have in service-footer.partial.  Please
refactor
> to not duplicate.

Done.

https://codereview.appspot.com/6724059/diff/1/app/templates/service.handlebars#newcode20
app/templates/service.handlebars:20: <div class="filter-control">
On 2012/10/18 21:19:07, gary.poster wrote:
> This is done with a dropdown in the mockup.  However, I prefer what
you have
> here (except that I expect the buttons need to be orange or somesuch).
  I'd run
> this past Nick/Jovan and get their take.

We want the buttons. The mockup is outdated. There is a new card for the
buttons style.

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

https://codereview.appspot.com/6724059/diff/1/app/views/environment.js#newcode101
app/views/environment.js:101: }
On 2012/10/18 21:19:07, gary.poster wrote:
> What are these changes to environment.js doing?  They seem completely
unrelated.
>   If this is work you did, please separate it.  If this is work from
another
> branch, please remove it.  Since this kind of thing has happened
before, I'll
> add that I try to always review the entire diff I am about to submit
before
> submitting it, to make sure I don't see anything that shouldn't be
there for one
> reason or another, or if I've missed anything.  I suggest adding this
to your
> process, if it is not there already.

I have no idea why this thing is here. Maybe it is due to the recent
trunk dance we had... and it may confused lbox? This code is in the
trunk anyway. If I remove it from here, I will remove it from the trunk,
right?

https://codereview.appspot.com/6724059/diff/1/app/views/environment.js#newcode1201
app/views/environment.js:1201: this.set('translate',
evt.translate.slice(0));
On 2012/10/18 21:19:07, gary.poster wrote:
> I didn't comment on any of the changes above.  Everything in this file
should
> go, I think.  It doesn't belong in this branch, one way or the other.

Ditto above

https://codereview.appspot.com/6724059/diff/1/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/6724059/diff/1/app/views/service.js#newcode387
app/views/service.js:387: this.renderable_charm(service.get('charm'),
app)),
On 2012/10/18 21:19:07, gary.poster wrote:
> you get the charm info below in line 390.  This is a decent amount of
work--much
> more than an attribute access, for instance.  Please stash the
renderable charm
> in a variable and then use it here and in line 390.

Done.

https://codereview.appspot.com/6724059/diff/1/app/views/service.js#newcode569
app/views/service.js:569: this.renderable_charm(service.get('charm'),
app)),
On 2012/10/18 21:19:07, gary.poster wrote:
> used again in line 579.  stash and reuse.

Done.

https://codereview.appspot.com/6724059/diff/1/app/views/service.js#newcode648
app/views/service.js:648: this.renderable_charm(service.get('charm'),
app)),
On 2012/10/18 21:19:07, gary.poster wrote:
> Used again in line 651.  Stash and reuse.

Done.

https://codereview.appspot.com/6724059/diff/1/app/views/service.js#newcode820
app/views/service.js:820: this.renderable_charm(service.get('charm'),
app)),
On 2012/10/18 21:19:07, gary.poster wrote:
> Used again in line 822.  Stash and reuse.

Done.

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

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode844
lib/views/stylesheet.less:844: .juju-service-info-container-bottom-menu
{
On 2012/10/18 21:19:07, gary.poster wrote:
> The rest of the file uses four space indents.  When in Rome, do as the
Romans.
> Don't make a file's stylistic conventions internally inconsistent.
Similarly,
> don't change a file's stylistic conventions within without discussion,
and if
> you do change them, change the whole file to match the new style.

> I have a branch that fixes this, and resolves a conflict with trunk.

Not on purpose. My formatter wasn't well configured. Sorry about that.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode880
lib/views/stylesheet.less:880: height: 111px;
On 2012/10/18 21:19:07, gary.poster wrote:
> This makes the area too big: the total height should be 111px, and
this makes it
> 38 + 111 = 149px.  I have corrected in my branch.

Done.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode884
lib/views/stylesheet.less:884: font-style: regular;
On 2012/10/18 21:19:07, gary.poster wrote:
> This is not a valid font-style.  I think you are looking for "normal."
  It is
> still unnecessary.  I have removed it in my branch.

Done.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode885
lib/views/stylesheet.less:885: font-size: 22px; fill: #292929;
On 2012/10/18 21:19:07, gary.poster wrote:
> I think you want color.  Fixed in my branch.

Done.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode889
lib/views/stylesheet.less:889: padding-top: 18px;
On 2012/10/18 21:19:07, gary.poster wrote:
> The guidelines

(https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqS19SYWQ2MzU3cFU/edit)
> show 18 pixels line height for the main name, not 18 pixels of space.
However,
> just eyeballing the image, that's clearly not what's going on--it's
closer to 30
> pixels.  I removed this and gave a height of 28 to the first child,
which seemed
> to approximate the image better.

Done.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode892
lib/views/stylesheet.less:892: font-size: 16px; fill: #6a737b;
On 2012/10/18 21:19:07, gary.poster wrote:
> fill only does something for svg, I believe.  You want color.  Fixed
in my
> branch.

Done.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode900
lib/views/stylesheet.less:900: background:
url(/juju-ui/assets/images/tab_div.png) repeat;
On 2012/10/18 21:19:07, gary.poster wrote:
> Can't this just be a bottom border?  Maybe not, but the "active" one
below
> really seems like it ought to be a border.

I should use the assets. These are the assets that I have:
https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqTjdsc3Q4TmdnTE0/edit

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode905
lib/views/stylesheet.less:905: font-style: medium;
On 2012/10/18 21:19:07, gary.poster wrote:
> There is no "medium" font-style.  I'm not sure what you were after.  I
deleted
> it.

I was just following the
https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqS19SYWQ2MzU3cFU/edit

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode906
lib/views/stylesheet.less:906: font-size: 12px; fill: #dd4814;
On 2012/10/18 21:19:07, gary.poster wrote:
> Same problem with fill.  You already have color.  deleted.

Done.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode920
lib/views/stylesheet.less:920: background:
url(/juju-ui/assets/images/tab_marker.png) repeat;
On 2012/10/18 21:19:07, gary.poster wrote:
> Is that image really more than a solid color?  it doesn't look like
it.

I should use the assets. These are the assets that I have:
https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqTjdsc3Q4TmdnTE0/edit

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newcode957
lib/views/stylesheet.less:957: font-family: @font-family;
On 2012/10/18 21:19:07, gary.poster wrote:
> I have a feeling that all the many uses of @font-family are
unnecessary, now
> that we have it in the body, but I didn't feel like digging in and
verifying.
> Do it if you like.

You are right. Thanks.

https://codereview.appspot.com/6724059/

-- 
https://code.launchpad.net/~tveronezi/juju-gui/service-header/+merge/130243
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~tveronezi/juju-gui/service-header into lp:juju-gui.