Add a loading messages and animation.

Add a loading messages and animation.



https://code.launchpad.net/~benji/juju-gui/bug-1110515-loading-message/+merge/148243

(do not edit description out of merge proposal)

[gary.poster, 2013-02-12 13:46:39]

Looks like good progress.

As I mentioned on IRC, the JS is still the right way forward I think.  The GIF
you gave uses rays rather than circles, different than what we have been told is
the Suru theme, and it is also 6 times the K weight of the minimized JS, if FF
is to be believed.

Also, in non-chrome browsers, we should only show the spinner box once the "your
browser is not supported" message has been shown, if I remember the flow diagram
properly.

Finally, the wording for the spinner should begin as something like "Loading the
Juju GUI". "Trying to connect to the Juju environment" will be appropriate when
we are actually connecting, as shown in the flow diagram.

Gary

https://codereview.appspot.com/7314082/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7314082/diff/1/app/index.html#newcode62
app/index.html:62: Trying to connect to the Juju environment.</span>
Initially this is about loading JS.  It's only about connecting once the app has
started.  The login shouldn't show until we've connected.

[benji, 2013-02-12 21:37:34]

Please take a look.

[benji, 2013-02-12 21:52:54]

Please take a look.

[gary.poster, 2013-02-12 21:55:40]

These comments were from an earlier version.  not much to them.  looking at the
newer version and doing some qa

https://codereview.appspot.com/7314082/diff/4001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7314082/diff/4001/app/index.html#newcode159
app/index.html:159: if (target.style !== undefined) {
interesting approach.  worth sharing?

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom
File test/test_startup.js.bottom (right):

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:4: // The behavior of the startTheApp function of
displaying the loding
loading

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:77: go('Browser that does not exist, and therefore
is not supported');
heh :-)

[bac, 2013-02-12 21:56:31]

Looks good except for some typos.  Thanks!

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom
File test/test_startup.js.bottom (right):

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:4: // The behavior of the startTheApp function of
displaying the loding
typo: loading

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:5: // message is emulated here, but the real
startTheApp funciton is
typo: function

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:65: // The loading message has not had it's default
style touched yet.
typo: its

https://codereview.appspot.com/7314082/diff/4001/test/test_startup.js.bottom#...
test/test_startup.js.bottom:74: it('will show a loading message after any
browser wanting', function() {
wanting?  did you mean 'waiting'?  otherwise i don't understand.

oh, warning?

[gary.poster, 2013-02-12 23:08:08]

Very nice Benji.  Thank you.  Especially good job with the testing in this and
the previous branch.

Land with changes.

I would strongly prefer that you follow up with the change I request in
index.html.  If you feel the need to defer for some reason, please file a bug.

This does not implement the exact UX that Nick gave in the flowchart, and that I
mentioned to you in my previous review.  I'm guessing it was intentional. :-) 
Specifically, I mentioned changing the message to describe the fact that we are
connecting, once we have loaded the JS and the app has started.  I believe you
may dislike this, and your good reasons might convince me, but please
specifically bring them up to both me and Nick.  I would be OK with you landing
this without that change, and then bringing it up with Nick in the UX review
lane.  Note that I had a conversation with Nick about this specific topic before
the branch.  That means both that I have some sympathy with what I believe to be
your position, and that the flowchart was done with consideration.

Thanks

Gary

https://codereview.appspot.com/7314082/diff/6007/Makefile
File Makefile (right):

https://codereview.appspot.com/7314082/diff/6007/Makefile#newcode312
Makefile:312: ln -sf "$(PWD)/app/assets/javascripts" build-$(1)/juju-ui/assets/
An unfortunate aspect of the fact that we don't explicitly set up dependencies
for these links is that reusing an old branch will not create what we need.  I
said that make prod was not working for me: I simply needed to make clean &&
make prod.  I suggest that you warn everyone about this when your branch lands.

https://codereview.appspot.com/7314082/diff/6007/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7314082/diff/6007/app/index.html#newcode203
app/index.html:203: YUI(GlobalConfig).use(['juju-gui'], function(Y) {
Actually, when qa-ing this branch, I realized that the related one was
problematic.  While we wait for the user to click "continue with the current
browser," we are not loading YUI JS.  We could be.  I think we could simply make
startTheApp global, either by omitting var or my explicitly attaching it to
window to make the point that it is what you want.

YUI(GlobalConfig).use('juju-gui'], function(Y) {
  window.startTheApp = function() {
    app = new Y.juju.....
    // ...
    app.activateHotkeys();
  };
});

That would be a significant improvement, I think.

https://codereview.appspot.com/7314082/diff/6007/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/7314082/diff/6007/app/views/charm-panel.js#new...
app/views/charm-panel.js:749: // Create a 'ghost' service to represent what will
be deployed.
huh?

Please make sure that this isn't really part of your branch. :-) I think it is
just a rietveld artifact of the fact that you made this proposal yesterday and
then merged trunk.

[benji, 2013-02-13 16:39:23]

> I would strongly prefer that you follow up with the change I request
> in index.html.  If you feel the need to defer for some reason, please
> file a bug.

Done. See the in-line comment for more details.

> This does not implement the exact UX that Nick gave in the flowchart,
> and that I mentioned to you in my previous review.

As discussed on IRC, reading this made me realize that this is what you
meant earlier.  I did not realize that the flow chart meant this.  To
clarify: there are to be *two* loading messages: one that says that the
application is loading and one that says that we are connecting to the
environment.  That is what I have now implemented.

https://codereview.appspot.com/7314082/diff/6007/Makefile
File Makefile (right):

https://codereview.appspot.com/7314082/diff/6007/Makefile#newcode312
Makefile:312: ln -sf "$(PWD)/app/assets/javascripts" build-$(1)/juju-ui/assets/
On 2013/02/12 23:08:08, gary.poster wrote:
> An unfortunate aspect of the fact that we don't explicitly set up dependencies
> for these links is that reusing an old branch will not create what we need.  I
> said that make prod was not working for me: I simply needed to make clean &&
> make prod.  I suggest that you warn everyone about this when your branch
lands.

I'm glad you figured it out.  Yeah, I'll email the list when this lands.

https://codereview.appspot.com/7314082/diff/6007/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7314082/diff/6007/app/index.html#newcode203
app/index.html:203: YUI(GlobalConfig).use(['juju-gui'], function(Y) {
On 2013/02/12 23:08:08, gary.poster wrote:
> Actually, when qa-ing this branch, I realized that the related one was
> problematic.  While we wait for the user to click "continue with the current
> browser," we are not loading YUI JS.

I have rearranged the code so this should work now.  I did extensive testing
with an artificially slowed-down network to verify the behavior.

I also added early loading of the hatched background image which significantly
improved the visuals in a slow-loading situation.

https://codereview.appspot.com/7314082/diff/6007/app/views/charm-panel.js
File app/views/charm-panel.js (right):

https://codereview.appspot.com/7314082/diff/6007/app/views/charm-panel.js#new...
app/views/charm-panel.js:749: // Create a 'ghost' service to represent what will
be deployed.
On 2013/02/12 23:08:08, gary.poster wrote:
> huh?
> 
> Please make sure that this isn't really part of your branch. :-) 

Nope, I'm not sure where it came from.

> I think it is
> just a rietveld artifact of the fact that you made this proposal yesterday and
> then merged trunk.

Maybe, but that makes me even more disappointed in lbox.

[benji, 2013-02-13 16:46:28]

*** Submitted:

Add a loading messages and animation.

R=gary.poster, bac
CC=
https://codereview.appspot.com/7314082

[benji, 2013-02-13 17:11:28]

*** Submitted:

Add a loading messages and animation.

R=
CC=
https://codereview.appspot.com/7314082