Service view header should match design doc

Service view header should match design doc

The service view header should match visual design document.
(https://docs.google.com/a/canonical.com/file/d/0B6l8lFdCRvtqS19SYWQ2MzU3cFU/edit)

https://code.launchpad.net/~tveronezi/juju-gui/service-header/+merge/130243

(do not edit description out of merge proposal)

[thiago, 2012-10-17 22:11:30]

Please take a look.

[gary.poster, 2012-10-18 21:19:07]

Hi Thiago.  This does look a lot better, you are right.  I have a lot of
comments, below.  I also made a branch to try and help out with some, but not
all, of the CSS comments.  feel free to merge or simply peruse and cherrypick,
as you wish.  lp:~gary/juju-gui/service-header

Gary

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

https://codereview.appspot.com/6724059/diff/1/app/templates/service-header.pa...
app/templates/service-header.partial:15: <a href="{{href}}">{{title}}</a>
These are supposed to be orange if the tab is active.

https://codereview.appspot.com/6724059/diff/1/app/templates/service-header.pa...
app/templates/service-header.partial:17: <div {{#if
active}}class="active"{{/if}}>
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.

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.handlebar...
app/templates/service.handlebars:11: <div>
Thanks for cleaning up the tabs.

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.handlebar...
app/templates/service.handlebars:16: </a>
This duplicates code that you have in service-footer.partial.  Please refactor
to not duplicate.

https://codereview.appspot.com/6724059/diff/1/app/templates/service.handlebar...
app/templates/service.handlebars:20: <div class="filter-control">
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.

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#newcod...
app/views/environment.js:101: }
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.

https://codereview.appspot.com/6724059/diff/1/app/views/environment.js#newcod...
app/views/environment.js:1201: this.set('translate', evt.translate.slice(0));
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.

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)),
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.

https://codereview.appspot.com/6724059/diff/1/app/views/service.js#newcode569
app/views/service.js:569: this.renderable_charm(service.get('charm'), app)),
used again in line 579.  stash and reuse.

https://codereview.appspot.com/6724059/diff/1/app/views/service.js#newcode648
app/views/service.js:648: this.renderable_charm(service.get('charm'), app)),
Used again in line 651.  Stash and reuse.

https://codereview.appspot.com/6724059/diff/1/app/views/service.js#newcode820
app/views/service.js:820: this.renderable_charm(service.get('charm'), app)),
Used again in line 822.  Stash and reuse.

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#newco...
lib/views/stylesheet.less:844: .juju-service-info-container-bottom-menu {
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.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:880: height: 111px;
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.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:884: font-style: regular;
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.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:885: font-size: 22px; fill: #292929;
I think you want color.  Fixed in my branch.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:889: padding-top: 18px;
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.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:892: font-size: 16px; fill: #6a737b;
fill only does something for svg, I believe.  You want color.  Fixed in my
branch.

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

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:905: font-style: medium;
There is no "medium" font-style.  I'm not sure what you were after.  I deleted
it.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:906: font-size: 12px; fill: #dd4814;
Same problem with fill.  You already have color.  deleted.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:920: background:
url(/juju-ui/assets/images/tab_marker.png) repeat;
Is that image really more than a solid color?  it doesn't look like it.

https://codereview.appspot.com/6724059/diff/1/lib/views/stylesheet.less#newco...
lib/views/stylesheet.less:957: font-family: @font-family;
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.

[thiago, 2012-10-19 13:57:27]

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

https://codereview.appspot.com/6724059/diff/1/app/templates/service-header.pa...
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.pa...
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.handlebar...
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.handlebar...
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.handlebar...
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#newcod...
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#newcod...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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.

[thiago, 2012-10-19 14:18:05]

Please take a look.

[hazmat, 2012-10-19 19:36:04]

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

https://codereview.appspot.com/6724059/diff/5002/app/views/service.js#newcode655
app/views/service.js:655: charm: renderableCharm}
the charm is referenced by id only in the templates, so all of these only need
 charm_id: service.get('charm')

ie. the renderable_charm call here is extraneous.

https://codereview.appspot.com/6724059/diff/5002/app/views/service.js#newcode822
app/views/service.js:822: var renderableCharm =
this.renderable_charm(service.get('charm'),
renderableCharm/renderedCharm

these lines could be removed afaics, and made internal to the getServiceTabs and
since its only needs app.getModelURL(this.get('charm')) the call to
renderable_charm can be omitted.

[thiago, 2012-10-19 20:23:40]

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

https://codereview.appspot.com/6724059/diff/5002/app/views/service.js#newcode655
app/views/service.js:655: charm: renderableCharm}
On 2012/10/19 19:36:04, hazmat wrote:
> the charm is referenced by id only in the templates, so all of these only need
>  charm_id: service.get('charm')
> 
> ie. the renderable_charm call here is extraneous.

Done.

https://codereview.appspot.com/6724059/diff/5002/app/views/service.js#newcode822
app/views/service.js:822: var renderableCharm =
this.renderable_charm(service.get('charm'),
On 2012/10/19 19:36:04, hazmat wrote:
> renderableCharm/renderedCharm
> 
> these lines could be removed afaics, and made internal to the getServiceTabs
and
> since its only needs app.getModelURL(this.get('charm')) the call to
> renderable_charm can be omitted.

Done.

[thiago, 2012-10-19 20:24:18]

Please take a look.

[hazmat, 2012-10-19 20:26:18]

looks good