Landscape Badges

Landscape Badges

This branch includes badges for landscape in:

	the environment view
	service views (single badges at top)
	unit views

It is currently very ugly, assets don't fit, match the designs very well
(pallet or size)  Additionally I had to generate smaller versions for the 
service view with various amounts of units. To see these goto the unit
page and test with 20, 40, 100, 300 units.

https://code.launchpad.net/~bcsaller/juju-gui/landscape-badges/+merge/151629

(do not edit description out of merge proposal)

[bcsaller, 2013-03-04 22:10:56]

Please take a look.

[gary.poster, 2013-03-04 23:02:17]

Graphic thoughts:

When you have a service with both icons, the rotating image is show in the
environment but not in the service view.  It should be used there too.

The rotating image looks clipped on the bottom for me.

I don't think the small images are the intent.  The mockups showed the boxes
being bigger.  I think you ought to discuss this with Nick though.

Thank you!  Hopefully this will be fast to resolve with the designers.  I'll
look at the code later.

Gary

[gary.poster, 2013-03-04 23:13:30]

One other thing. It would be great if you could go ahead and have this branch
set up in a charm and send the URL to Nick so that he can look at it as soon as
possible tomorrow.

Thanks again,
Gary

On Mar 4, 2013, at 6:02 PM, gary.poster@canonical.com wrote:

> Graphic thoughts:
> 
> When you have a service with both icons, the rotating image is show in
> the environment but not in the service view.  It should be used there
> too.
> 
> The rotating image looks clipped on the bottom for me.
> 
> I don't think the small images are the intent.  The mockups showed the
> boxes being bigger.  I think you ought to discuss this with Nick though.
> 
> Thank you!  Hopefully this will be fast to resolve with the designers.
> I'll look at the code later.
> 
> Gary
> 
> 
> 
> https://codereview.appspot.com/7415055/

[bcsaller, 2013-03-05 18:49:31]

Please take a look.

[gary.poster, 2013-03-06 05:12:04]

LGTM with changes.  Thank you!

Please push everyone to get one other person to review this so you can land it
Wednesday. 

Gary

https://codereview.appspot.com/7415055/diff/5001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/7415055/diff/5001/app/config-debug.js#newcode10
app/config-debug.js:10: socket_url: 'ws://192.168.122.1:8081/ws',
Please omit this!

https://codereview.appspot.com/7415055/diff/5001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7415055/diff/5001/app/views/topology/service.j...
app/views/topology/service.js:644:
'/juju-ui/assets/images/landscape_restart_round.png';
I don't love the fact that we have to choose which is more important for the
user to see.  I'm OK with this to land but may revisit it with Nick later.

https://codereview.appspot.com/7415055/diff/5001/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/7415055/diff/5001/app/views/utils.js#newcode1132
app/views/utils.js:1132: Y.Handlebars.registerHelper('landscapeBadge', function(
Cool.

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

https://codereview.appspot.com/7415055/diff/5001/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:450: padding: 15px;
We no longer need this change, right?  If so, please remove.

https://codereview.appspot.com/7415055/diff/5001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7415055/diff/5001/test/test_environment_view.j...
test/test_environment_view.js:523: wordpress['landscape-security-upgrades'] =
false;
I'd like it if we could actually assert that this has drawn a badge, the same
way we are doing with the landscape controls, below.

[bcsaller, 2013-03-06 05:47:44]

Thanks for the review, I'll propose again for the next reviewer.

https://codereview.appspot.com/7415055/diff/5001/app/config-debug.js
File app/config-debug.js (right):

https://codereview.appspot.com/7415055/diff/5001/app/config-debug.js#newcode10
app/config-debug.js:10: socket_url: 'ws://192.168.122.1:8081/ws',
On 2013/03/06 05:12:05, gary.poster wrote:
> Please omit this!

Eek. was testing it on IE at one point, fixed :)

I actually just went in and made a change we'll need tomorrow anyway, if no
socket_url is provided 
assemble one from the Location object. If socket_port
is provided use that rather than the port we are talking to the web server on.
That should support all our common testing (by leaving 8081 in the config files)
but support
getting the origin right.

https://codereview.appspot.com/7415055/diff/5001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/7415055/diff/5001/app/views/topology/service.j...
app/views/topology/service.js:644:
'/juju-ui/assets/images/landscape_restart_round.png';
On 2013/03/06 05:12:05, gary.poster wrote:
> I don't love the fact that we have to choose which is more important for the
> user to see.  I'm OK with this to land but may revisit it with Nick later.

It is very possible we'll have some badge that indicates both states or uses a
gentler animation in the future, its easy enough to swap in now.

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

https://codereview.appspot.com/7415055/diff/5001/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:450: padding: 15px;
On 2013/03/06 05:12:05, gary.poster wrote:
> We no longer need this change, right?  If so, please remove.

We don't. I should have really reviewed my own diff. Thanks.

https://codereview.appspot.com/7415055/diff/5001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7415055/diff/5001/test/test_environment_view.j...
test/test_environment_view.js:523: wordpress['landscape-security-upgrades'] =
false;
On 2013/03/06 05:12:05, gary.poster wrote:
> I'd like it if we could actually assert that this has drawn a badge, the same
> way we are doing with the landscape controls, below.

I now check that a badge is rendered on the service.

[bcsaller, 2013-03-06 05:49:38]

Please take a look.

[jeff.pihach, 2013-03-06 14:30:41]

LGTM with trivial

Thanks this looks good!

https://codereview.appspot.com/7415055/diff/16001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7415055/diff/16001/app/index.html#newcode243
app/index.html:243: juju_config.socket_url = "ws://" + loc.hostname + ':' + port
+ '/ws';
We should probably allow a specification of the protocol, ws or wss

https://codereview.appspot.com/7415055/diff/16001/app/views/landscape.js
File app/views/landscape.js (right):

https://codereview.appspot.com/7415055/diff/16001/app/views/landscape.js#newc...
app/views/landscape.js:142: getLandscapeBadge: function(model, intent, hint) {
I think this is missing the yuidoc params

[bcsaller, 2013-03-06 15:07:13]

Thanks for the reviews.

I think the ws url builder does what we need now as indicated below.

https://codereview.appspot.com/7415055/diff/16001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7415055/diff/16001/app/index.html#newcode243
app/index.html:243: juju_config.socket_url = "ws://" + loc.hostname + ':' + port
+ '/ws';
On 2013/03/06 14:30:41, jeff.pihach wrote:
> We should probably allow a specification of the protocol, ws or wss

I now allow specification of socket_protocol, defaulting to 
'ws'

https://codereview.appspot.com/7415055/diff/16001/app/views/landscape.js
File app/views/landscape.js (right):

https://codereview.appspot.com/7415055/diff/16001/app/views/landscape.js#newc...
app/views/landscape.js:142: getLandscapeBadge: function(model, intent, hint) {
On 2013/03/06 14:30:41, jeff.pihach wrote:
> I think this is missing the yuidoc params

added.

[bcsaller, 2013-03-06 15:12:45]

*** Submitted:

Landscape Badges

This branch includes badges for landscape in:

	the environment view
	service views (single badges at top)
	unit views

It is currently very ugly, assets don't fit, match the designs very well
(pallet or size)  Additionally I had to generate smaller versions for the 
service view with various amounts of units. To see these goto the unit
page and test with 20, 40, 100, 300 units.

R=gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/7415055