yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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.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>
These are supposed to be orange if the tab is active.

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}}>
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.handlebars#oldcode11
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.handlebars#newcode16
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.handlebars#newcode20
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#newcode101
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#newcode1201
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#newcode844
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#newcode880
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#newcode884
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#newcode885
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#newcode889
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#newcode892
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#newcode900
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#newcode905
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#newcode906
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#newcode920
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#newcode957
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.

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.