Code review comment for lp:~benji/juju-gui/bug-1104105-browser-compatability-warning-3

Hi Benji. Interesting test approach that is cool. Will think about it
more.

I raised the concern on IRC last week that make prod appears to be
broken. Here are some other review comments. I'll re-review when you
have these addressed and make prod fixed.

Thank you

Gary

https://codereview.appspot.com/7299071/diff/3001/Makefile
File Makefile (right):

https://codereview.appspot.com/7299071/diff/3001/Makefile#newcode523
Makefile:523: server spritegen test test-debug test-prod undocumented \
test-prep too, yeah?

https://codereview.appspot.com/7299071/diff/3001/app/index.html
File app/index.html (right):

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode41
app/index.html:41: Your browser not fully supported.</span>
add an "is" in there please.

On IE 10 this is two lines, even without the is, which is a shame. It
would be nice if the panel were wider. I might mess with that.

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode50
app/index.html:50: <a href="http://www.google.com/chrome">Chrome</a> to
be fully
I think we ought to say Firefox here too. Maybe we wait for CI for
that. If so, please make a bug about this ("Firefox is not regularly
tested" with description mentioning that CI is fix for this bug, and
that this message should be updated when it is).

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode113
app/index.html:113: startTheApp = function() {
Unless someone corrects me, I'll assert that is a matter of good
practice to always use var declarations, even at the top level where
they technically are equivalent to what you have written here. Unless
someone argues against them, please add var statements here and below.
(see pastebin from next comment)

https://codereview.appspot.com/7299071/diff/3001/app/index.html#newcode150
app/index.html:150: </script>
This ordering does not accomplish the workflow we want, described in the
graphic assets. The browser warning should be shown before we load the
JS. I think this is one approach to get what we want:
http://pastebin.ubuntu.com/1636315/ . I suspect there are more elegant
ones.

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

https://codereview.appspot.com/7299071/diff/3001/lib/views/stylesheet.less#newcode1433
lib/views/stylesheet.less:1433: width: 300px;
I'd like to see what this looks like expanded a bit. The goal would be
to have the text fit in a single line at the top, while still looking
good for login.

https://codereview.appspot.com/7299071/