Merge lp://staging/~bcsaller/juju-gui/service-icon into lp://staging/juju-gui/experimental

Reviewers: mp+174276_code.launchpad.net,

Message:
Please take a look.

Description:
Service Icon + Misc Fix

Include service icon on inspector pane.
Fix status bar label ordering in DOM so they always appear.
Temp fix around long unit lists with scrolling overflow.

https://code.launchpad.net/~bcsaller/juju-gui/service-icon/+merge/174276

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/11181043/

Affected files:
   A [revision details]
   M app/assets/javascripts/d3.status.js
   M app/templates/serviceOverview.handlebars
   M app/views/service.js
   M lib/views/stylesheet.less

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/templates/serviceOverview.handlebars
=== modified file 'app/templates/serviceOverview.handlebars'
--- app/templates/serviceOverview.handlebars 2013-07-11 16:53:03 +0000
+++ app/templates/serviceOverview.handlebars 2013-07-11 17:53:13 +0000
@@ -1,6 +1,6 @@
  <div class="content">
    <div class="service-charm">
- <div class="charm-icon">
+ <div class="charm-icon" data-bind="icon">
    </div>
    <span data-bind="displayName" class="service-name"></span><br />
    <span data-bind="charm"></span>

Index: app/views/service.js
=== modified file 'app/views/service.js'
--- app/views/service.js 2013-07-11 16:59:28 +0000
+++ app/views/service.js 2013-07-11 17:53:13 +0000
@@ -1433,6 +1433,15 @@
                bar.update(value);
              }
            },
+ icon: {
+ 'update': function(node, value) {
+ var icon = Y.one(node).one('img');
+ if (!icon) {
+ icon = Y.one(node).append('<img>');
+ }
+ icon.set('src', value);
+ }
+ },
            units: {
              depends: ['aggregated_status'],
              'update': function(node, value) {

Index: lib/views/stylesheet.less
=== modified file 'lib/views/stylesheet.less'
--- lib/views/stylesheet.less 2013-07-11 05:14:50 +0000
+++ lib/views/stylesheet.less 2013-07-11 19:21:16 +0000
@@ -599,6 +599,11 @@
          background-color: lighten(@pending-color, 20%);
      }
  }
+
+.overview-unit-list {
+ max-height: 11em;
+ overflow-x: scroll;
+}
  .unit (@min_height: 20px, @border_radius:3px) {
      .create-border-radius(@border_radius);
      padding: 5px;

Index: app/assets/javascripts/d3.status.js
=== modified file 'app/assets/javascripts/d3.status.js'
--- app/assets/javascripts/d3.status.js 2013-07-11 05:14:50 +0000
+++ app/assets/javascripts/d3.status.js 2013-07-11 19:15:02 +0000
@@ -184,7 +184,7 @@
        // Enter/Update/Exit Bars
        rects
        .enter()
- .append('rect')
+ .insert('rect', 'text')
        .each(function(d) {
              var node = d3.select(this);
              node.classed(d.key, true);

Code LGTM.

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

https://codereview.appspot.com/11181043/diff/1/app/views/service.js#newcode1440
app/views/service.js:1440: icon = Y.one(node).append('<img>');
If we could get the category icon that would be preferable. Is that an
easy option? If not, maybe file a bug?

https://codereview.appspot.com/11181043/

On 2013/07/11 19:34:05, gary.poster wrote:
> Code LGTM.

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

https://codereview.appspot.com/11181043/diff/1/app/views/service.js#newcode1440
> app/views/service.js:1440: icon = Y.one(node).append('<img>');
> If we could get the category icon that would be preferable. Is that
an easy
> option? If not, maybe file a bug?

Actually it looks like there might be a few problems with the icon
handling though they don't come from this branch. Ideally asking for the
icon for the service should return the best match. This could work if
service.get('icon') operated as a read through cache of the best match
from the charm browser. However it looks like icon is statically
assigned only when ghosted through the GUI. This means CLI deployed
environment won't have icons bound to them. I'd suggest that we leave
this in place for now and that the longer term interaction of GUI models
with the charm store API be a short round table meeting at the sprint.
Is that ok with you?

https://codereview.appspot.com/11181043/

On 2013/07/11 19:56:39, benjamin.saller wrote:
> On 2013/07/11 19:34:05, gary.poster wrote:
> > Code LGTM.
> >
> > https://codereview.appspot.com/11181043/diff/1/app/views/service.js
> > File app/views/service.js (right):
> >
> >

https://codereview.appspot.com/11181043/diff/1/app/views/service.js#newcode1440
> > app/views/service.js:1440: icon = Y.one(node).append('<img>');
> > If we could get the category icon that would be preferable. Is that
an easy
> > option? If not, maybe file a bug?

> Actually it looks like there might be a few problems with the icon
handling
> though they don't come from this branch. Ideally asking for the icon
for the
> service should return the best match. This could work if
service.get('icon')
> operated as a read through cache of the best match from the charm
browser.
> However it looks like icon is statically assigned only when ghosted
through the
> GUI. This means CLI deployed environment won't have icons bound to
them. I'd
> suggest that we leave this in place for now and that the longer term
interaction
> of GUI models with the charm store API be a short round table meeting
at the
> sprint. Is that ok with you?

Yeah, I was planning that. Could you add this as a note to
https://bugs.launchpad.net/juju-gui/+bug/1200008 and mark this locally
as an XXX?

Thanks

https://codereview.appspot.com/11181043/

LGTM, this works for reviewed charms with an icon. As noted, we can work
out a better way to pull the icon logic into a better shared location
apart from this.

https://codereview.appspot.com/11181043/

*** Submitted:

Service Icon + Misc Fix

Include service icon on inspector pane.
Fix status bar label ordering in DOM so they always appear.

R=gary.poster, benjamin.saller, rharding
CC=
https://codereview.appspot.com/11181043

https://codereview.appspot.com/11181043/