Converted service and unit views to use promises

Converted service and unit views to use promises

In addition to converting the unit and service views
to use promises: 

If the user is viewing a service and
that service is no longer available it will redirect 
them to the environment with a notification.

If the user is viewing a unit and that unit is no longer
available the user will be redirected to the service.

If the user is viewing a unit and that units service
is no longer available then the user will be redirected
to the service.

https://code.launchpad.net/~hatch/juju-gui/view-promises/+merge/162205

(do not edit description out of merge proposal)

[jeff.pihach, 2013-05-02 18:23:08]

Please take a look.

[jeff.pihach, 2013-05-02 18:28:08]

Please take a look.

[gary.poster, 2013-05-02 19:08:31]

This is good code but a bad diff, I'm afraid.  Please take a look at my comments
and change as appropriate, but then re-propose a new version of the branch so it
is clearer what is actually part of your branch.

Thanks

Gary

https://codereview.appspot.com/9132043/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode519
app/app.js:519: self.showView('unit', options, { update: true });
If we're showing "please wait" then that should be the first time the view is
rendered, right?  So update: true would not be necessary.

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode520
app/app.js:520: }, 100);
You have such nice comments in _buildServiceView, but people are left alone up
here in show_unit? :-)

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode576
app/app.js:576: // only have it's config updated not re-rendered.
grammar police: it's -> its

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode578
app/app.js:578: self.showView(viewName, options, { update: true });
If we're showing "please wait" then that should be the first time the view is
rendered, right?  So update: true would not be necessary.

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode593
app/app.js:593: // while it is being viewed.
I don't understand why this helps. Could you clarify?

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode865
app/app.js:865: this.renderEnvironment = false;
I'd like this to be marked with an XXX, pointing to the change that we are
making to enable a registration of /:gui:/ for the environment view.

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode1027
app/app.js:1027: { path: '*', callbacks: 'toggleStaticViews'},
A fix for /:gui:/ will come in a later branch? Is there a card?

https://codereview.appspot.com/9132043/diff/2001/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/9132043/diff/2001/app/modules-debug.js#newcode168
app/modules-debug.js:168: requires: ['service-view-promise-support']
I thought we didn't need this now.

https://codereview.appspot.com/9132043/diff/2001/app/subapps/browser/views/ch...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/9132043/diff/2001/app/subapps/browser/views/ch...
app/subapps/browser/views/charm.js:459: this.get('renderTo').setHTML(tplNode);
Uh?  Are the changes in this file really yours?

https://codereview.appspot.com/9132043/diff/2001/app/subapps/browser/views/ed...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9132043/diff/2001/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:27: widgets.browser.IndicatorManager
these file changes are not yours also.  I might like a fresh submission so we
can be sure that this is not a reversion of something. :-/

https://codereview.appspot.com/9132043/diff/2001/app/subapps/browser/views/se...
File app/subapps/browser/views/search.js (right):

https://codereview.appspot.com/9132043/diff/2001/app/subapps/browser/views/se...
app/subapps/browser/views/search.js:19: widgets.browser.IndicatorManager,
Not your changes.

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

https://codereview.appspot.com/9132043/diff/2001/app/views/utils.js#newcode1366
app/views/utils.js:1366: // If there's a spinning indicator on the View then
make sure to hide
Not yours?

https://codereview.appspot.com/9132043/diff/2001/lib/views/browser/main.less
File lib/views/browser/main.less (right):

https://codereview.appspot.com/9132043/diff/2001/lib/views/browser/main.less#...
lib/views/browser/main.less:252: z-index: 1;
Not yours!

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

https://codereview.appspot.com/9132043/diff/2001/lib/views/stylesheet.less#ne...
lib/views/stylesheet.less:287: GUI itself). */
Not yours!

[jeff.pihach, 2013-05-02 19:57:01]

Thanks for the review, comments below and changes have been made to the review
with the proper diff.

https://codereview.appspot.com/9132043/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode519
app/app.js:519: self.showView('unit', options, { update: true });
On 2013/05/02 19:08:31, gary.poster wrote:
> If we're showing "please wait" then that should be the first time the view is
> rendered, right?  So update: true would not be necessary.

Done.

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode520
app/app.js:520: }, 100);
Additional comments added :)

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode576
app/app.js:576: // only have it's config updated not re-rendered.
On 2013/05/02 19:08:31, gary.poster wrote:
> grammar police: it's -> its

Done.

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode578
app/app.js:578: self.showView(viewName, options, { update: true });
On 2013/05/02 19:08:31, gary.poster wrote:
> If we're showing "please wait" then that should be the first time the view is
> rendered, right?  So update: true would not be necessary.

Done.

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode593
app/app.js:593: // while it is being viewed.
Additional comments added to the code

At this point the service view could be in loading state
or showing details but the service has become unavailable
or was never available. This calls a method on the view
to redirect to the environment and to create a notification

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode865
app/app.js:865: this.renderEnvironment = false;
On 2013/05/02 19:08:31, gary.poster wrote:
> I'd like this to be marked with an XXX, pointing to the change that we are
> making to enable a registration of /:gui:/ for the environment view.

Done.

https://codereview.appspot.com/9132043/diff/2001/app/app.js#newcode1027
app/app.js:1027: { path: '*', callbacks: 'toggleStaticViews'},
No card yet, Ben and I weren't sure that this router change would be necessary
come the new layout changes.

https://codereview.appspot.com/9132043/diff/2001/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/9132043/diff/2001/app/modules-debug.js#newcode168
app/modules-debug.js:168: requires: ['service-view-promise-support']
On 2013/05/02 19:08:31, gary.poster wrote:
> I thought we didn't need this now.

Done.