Code review - Issue 7314082: Add a loading messages and animation.https://codereview.appspot.com/2013-02-13T17:11:28+00:00rietveld
Message from unknown
2013-02-11T22:27:33+00:00benjiurn:md5:c71b2bebe38d970ff2060418be3b3c67
Message from gary.poster@canonical.com
2013-02-12T13:46:39+00:00gary.posterurn:md5:5f5f5eed03115be366575b141dfa4000
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.
Message from unknown
2013-02-12T21:37:31+00:00benjiurn:md5:efe74fc5e62cf9533f0e7f3fa75f1d8f
Message from benji.york@gmail.com
2013-02-12T21:37:34+00:00benjiurn:md5:078eea2936f67770625afc3fb9d44cab
Please take a look.
Message from unknown
2013-02-12T21:52:48+00:00benjiurn:md5:5388ec530a2bc1621bb3df7fd1217ba3
Message from benji.york@gmail.com
2013-02-12T21:52:54+00:00benjiurn:md5:0a328e14bcddc20b9c3a9eb1574b0981
Please take a look.
Message from gary.poster@canonical.com
2013-02-12T21:55:40+00:00gary.posterurn:md5:f81bf72f8d3b27d4c6d1f865ab249688
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#newcode4
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#newcode77
test/test_startup.js.bottom:77: go('Browser that does not exist, and therefore is not supported');
heh :-)
Message from bac@canonical.com
2013-02-12T21:56:31+00:00bacurn:md5:871b3a289e7893cab53d93088956ecfb
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#newcode4
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#newcode5
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#newcode65
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#newcode74
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?
Message from gary.poster@canonical.com
2013-02-12T23:08:08+00:00gary.posterurn:md5:f63efd92907fecf0b513fc96911cf35d
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#newcode749
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.
Message from unknown
2013-02-13T13:41:27+00:00benjiurn:md5:c4c499ff404f4277a32962dfe3870faa
Message from benji.york@gmail.com
2013-02-13T16:39:23+00:00benjiurn:md5:771fb1ce0994eb7871571d111592c5d5
> 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#newcode749
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.
Message from unknown
2013-02-13T16:40:31+00:00benjiurn:md5:e17eb0d7864b8b17e5af07c30b30978c
Message from unknown
2013-02-13T16:43:21+00:00benjiurn:md5:86909f3e583531e470437b0926b456e0
Message from benji.york@gmail.com
2013-02-13T16:46:28+00:00benjiurn:md5:2d237ed9c21eeecd292c2211e6abd2d6
*** Submitted:
Add a loading messages and animation.
R=gary.poster, bac
CC=
https://codereview.appspot.com/7314082
Message from unknown
2013-02-13T17:08:53+00:00benjiurn:md5:542d550d9c334508d83b26473c66ceb6
Message from benji.york@gmail.com
2013-02-13T17:11:28+00:00benjiurn:md5:3f66d78287825f822f2905bd3b869b7a
*** Submitted:
Add a loading messages and animation.
R=
CC=
https://codereview.appspot.com/7314082