Add Landscape bottom-bar controls

Add Landscape bottom-bar controls

Landscape controls added to the bottom of environment, service, and unit views.  This also involved, per UX check, moving the destroy and expose controls onthe service page to the top of the page.

https://code.launchpad.net/~makyo/juju-gui/env-links-1126546/+merge/151070

(do not edit description out of merge proposal)

[matthew.scott, 2013-02-27 22:29:06]

Please take a look.

[gary.poster, 2013-02-28 02:16:57]

LGTM with the addition of a few tests I mention, though I am concerned that Nick
may need you to increase the height of the top service menu to match
https://docs.google.com/a/canonical.com/file/d/0B1IM--9A1RkTdWNBcVdKMngzc0E/edit
.

Note to other reviewers: to actually qa this, you need
lp:~bcsaller/juju/delta-annotations instead of rapi-rollup.

This looks very nice.  Feel free to ignore my comment about "entity": once I saw
the app in action, I realized that the icon is in fact of a machine, so your
name makes more sense to me now than it did at first blush.

Thanks!

Gary

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

https://codereview.appspot.com/7417043/diff/1/app/views/utils.js#newcode386
app/views/utils.js:386: var machine = controls.one('.machine-control');
trivial: maybe this should be "entity" or something? It's only a machine if we
are on the unit page.

https://codereview.appspot.com/7417043/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7417043/diff/1/test/test_environment_view.js#n...
test/test_environment_view.js:523:
rebootItem.one('a').get('href').should.equal('http://host/foo+reboot');
Nice additions, but it would be good to test that the security update is hidden
and the main landscape icon has the right link, at least somewhere.  Perhaps in
a test of the utility function, rather than spread out among the view tests?

[bcsaller, 2013-02-28 15:21:25]

LGTM with some suggestions below. Take it or leave it.

https://codereview.appspot.com/7417043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7417043/diff/1/app/app.js#newcode624
app/app.js:624: querystring: req.query, landscape: this.landscape });
nitpick Maybe its own line.

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

https://codereview.appspot.com/7417043/diff/1/app/views/utils.js#newcode378
app/views/utils.js:378: utils.updateLandscapeBottomBar = function(landscape,
env, model, container,
Only because I used `intent` in getLandscapeURL I thought you'd somehow
overloaded it here, but it was a bad read on my part and you use it for
something different. In this case it might be 'scope' more than intent though.

https://codereview.appspot.com/7417043/diff/1/app/views/utils.js#newcode387
app/views/utils.js:387: var updates = controls.one('.updates-control');
Minor: If you add .hide() (its chainable) to the selection you can drop the else
{ hide } blocks below. Not sure if you like that style or not, I used it in the
menu though and thought the conditionals looked cleaner.

[matthew.scott, 2013-02-28 18:13:34]

Please take a look.

[bcsaller, 2013-02-28 18:16:31]

LGTM

I think the util method refactor came out nicely and the new test is a win.

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

https://codereview.appspot.com/7417043/diff/8001/app/views/utils.js#newcode385
app/views/utils.js:385: var logo = controls.one('.logo-tab i');
I think the version is easier to follow, thanks.

https://codereview.appspot.com/7417043/diff/8001/test/test_landscape.js
File test/test_landscape.js (right):

https://codereview.appspot.com/7417043/diff/8001/test/test_landscape.js#newco...
test/test_landscape.js:130: it('should build the bottom-bar properly',
function() {
Thanks for adding this.

[matthew.scott, 2013-02-28 18:16:47]

Thanks for the reviews.  I talked with Nick this morning re: the top bar and we
agreed to add 4px.  Will get a re-confirmation on the UI with the added space.

https://codereview.appspot.com/7417043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7417043/diff/1/app/app.js#newcode624
app/app.js:624: querystring: req.query, landscape: this.landscape });
On 2013/02/28 15:21:26, bcsaller wrote:
> nitpick Maybe its own line. 

Done.

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

https://codereview.appspot.com/7417043/diff/1/app/views/utils.js#newcode378
app/views/utils.js:378: utils.updateLandscapeBottomBar = function(landscape,
env, model, container,
On 2013/02/28 15:21:26, bcsaller wrote:
> Only because I used `intent` in getLandscapeURL I thought you'd somehow
> overloaded it here, but it was a bad read on my part and you use it for
> something different. In this case it might be 'scope' more than intent though.

Done.

https://codereview.appspot.com/7417043/diff/1/app/views/utils.js#newcode387
app/views/utils.js:387: var updates = controls.one('.updates-control');
On 2013/02/28 15:21:26, bcsaller wrote:
> Minor: If you add .hide() (its chainable) to the selection you can drop the
else
> { hide } blocks below. Not sure if you like that style or not, I used it in
the
> menu though and thought the conditionals looked cleaner.

Done.

https://codereview.appspot.com/7417043/diff/1/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/7417043/diff/1/test/test_environment_view.js#n...
test/test_environment_view.js:523:
rebootItem.one('a').get('href').should.equal('http://host/foo+reboot');
On 2013/02/28 02:16:57, gary.poster wrote:
> Nice additions, but it would be good to test that the security update is
hidden
> and the main landscape icon has the right link, at least somewhere.  Perhaps
in
> a test of the utility function, rather than spread out among the view tests?

Done.

[gary.poster, 2013-02-28 19:16:06]

LGTM.  Thank you!

gary

https://codereview.appspot.com/7417043/diff/8001/test/test_landscape.js
File test/test_landscape.js (right):

https://codereview.appspot.com/7417043/diff/8001/test/test_landscape.js#newco...
test/test_landscape.js:130: it('should build the bottom-bar properly',
function() {
On 2013/02/28 18:16:31, bcsaller wrote:
> Thanks for adding this.

+1

[goodspud, 2013-03-01 15:15:48]

LGTM Sign off from Design. Cheers!

[matthew.scott, 2013-03-01 15:26:21]

*** Submitted:

Add Landscape bottom-bar controls

Landscape controls added to the bottom of environment, service, and unit views. 
This also involved, per UX check, moving the destroy and expose controls onthe
service page to the top of the page.

R=gary.poster, bcsaller, goodspud
CC=
https://codereview.appspot.com/7417043