Update Chromoting login UI

Update Chromoting login UI

BUG=none
TEST=chromoting UI manual tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75417

[Sergey Ulanov, 2011-02-14 21:52:49]

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
File remoting/client/extension/chromoting_tab.html (right):

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:16: <div id="login_panel"
class="login_panel">
Alpha added login_panel for local login. It looks like your change is for Gaia
login. Maybe we need another panel for that.

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:17: <table cellspacing="3"
cellpadding="5" border="0"><tr><td valign="top"
use CSS instead of  border/cellpadding/cellspacing/bgcolor attributes. define
style in CSS instead of inline

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:19: <table align="center"
border="0" cellpadding="1" cellspacing="0">
table inside of table, why?

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:22: <font size="-1">Sign in with
your</font>
don't use <font>. Also I don't think we should insert image inside of text,
especially using tables - it is not very localizible and accessible

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:23: <table><tr>
Why do you need <table> here. just set position:float for the image.

[garykac, 2011-02-14 22:11:52]

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
File remoting/client/extension/chromoting_tab.html (right):

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:16: <div id="login_panel"
class="login_panel">
On 2011/02/14 21:52:49, sergeyu wrote:
> Alpha added login_panel for local login. It looks like your change is for Gaia
> login. Maybe we need another panel for that.

The changes to Alpha's pre-local login are from Alberto, and (as I understand
it) are Gaia login.  The local login screen is separate.

Both the chromoting dir login and the pre-local login use the same css styles
now.

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:17: <table cellspacing="3"
cellpadding="5" border="0"><tr><td valign="top"
On 2011/02/14 21:52:49, sergeyu wrote:
> use CSS instead of  border/cellpadding/cellspacing/bgcolor attributes. define
> style in CSS instead of inline

This formatting was all duplicated verbatim from GMail.

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:19: <table align="center"
border="0" cellpadding="1" cellspacing="0">
On 2011/02/14 21:52:49, sergeyu wrote:
> table inside of table, why?

Gmail.

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:22: <font size="-1">Sign in with
your</font>
On 2011/02/14 21:52:49, sergeyu wrote:
> don't use <font>. Also I don't think we should insert image inside of text,
> especially using tables - it is not very localizible and accessible

Again, this is from Gmail.

http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
remoting/client/extension/chromoting_tab.html:23: <table><tr>
On 2011/02/14 21:52:49, sergeyu wrote:
> Why do you need <table> here. just set position:float for the image.

layout copied from GMail.

[garykac, 2011-02-15 00:18:17]

On 2011/02/14 22:11:52, garykac wrote:
>
http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
> File remoting/client/extension/chromoting_tab.html (right):
> 
>
http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
> remoting/client/extension/chromoting_tab.html:16: <div id="login_panel"
> class="login_panel">
> On 2011/02/14 21:52:49, sergeyu wrote:
> > Alpha added login_panel for local login. It looks like your change is for
Gaia
> > login. Maybe we need another panel for that.
> 
> The changes to Alpha's pre-local login are from Alberto, and (as I understand
> it) are Gaia login.  The local login screen is separate.
> 
> Both the chromoting dir login and the pre-local login use the same css styles
> now.
> 
>
http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
> remoting/client/extension/chromoting_tab.html:17: <table cellspacing="3"
> cellpadding="5" border="0"><tr><td valign="top"
> On 2011/02/14 21:52:49, sergeyu wrote:
> > use CSS instead of  border/cellpadding/cellspacing/bgcolor attributes.
define
> > style in CSS instead of inline
> 
> This formatting was all duplicated verbatim from GMail.
> 
>
http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
> remoting/client/extension/chromoting_tab.html:19: <table align="center"
> border="0" cellpadding="1" cellspacing="0">
> On 2011/02/14 21:52:49, sergeyu wrote:
> > table inside of table, why?
> 
> Gmail.
> 
>
http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
> remoting/client/extension/chromoting_tab.html:22: <font size="-1">Sign in with
> your</font>
> On 2011/02/14 21:52:49, sergeyu wrote:
> > don't use <font>. Also I don't think we should insert image inside of text,
> > especially using tables - it is not very localizible and accessible
> 
> Again, this is from Gmail.
> 
>
http://codereview.chromium.org/6518013/diff/1/remoting/client/extension/chrom...
> remoting/client/extension/chromoting_tab.html:23: <table><tr>
> On 2011/02/14 21:52:49, sergeyu wrote:
> > Why do you need <table> here. just set position:float for the image.
> 
> layout copied from GMail.

I changed the appearance of the "local login" prompt to remove references to
"Google Account".  It now says "Sign in to remote host", but we'll want to
revisit this UI later.

[Sergey Ulanov, 2011-02-15 22:06:43]

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
File remoting/client/extension/client.js (right):

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:71: function logout_reload(form) {
use mixedCase identifiers in JS. logoutAndReload()?

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:86: function chromodir_login() {
maybe showDirectoryLogin()?

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:164: var errormsgDiv =
document.getElementById('errormsg_div');
errorMsgDiv or errorDiv

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:167:
errormsgDiv.appendChild(document.createTextNode(
I would write errormsgDiv.innerHTML = "...". It is much easier to read.

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/ma...
File remoting/client/extension/main.css (right):

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/ma...
remoting/client/extension/main.css:156: .gaia.le.lbl { font-family: Arial,
Helvetica, sans-serif; }
Do we really need this to be verbatim copy from gaia? I think it is better to
clean this up (e.g. don't specify font in 6 different places).

[garykac, 2011-02-16 18:43:06]

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
File remoting/client/extension/client.js (right):

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:71: function logout_reload(form) {
On 2011/02/15 22:06:43, sergeyu wrote:
> use mixedCase identifiers in JS. logoutAndReload()?

Done.

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:86: function chromodir_login() {
On 2011/02/15 22:06:43, sergeyu wrote:
> maybe showDirectoryLogin()?

Done.

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:164: var errormsgDiv =
document.getElementById('errormsg_div');
On 2011/02/15 22:06:43, sergeyu wrote:
> errorMsgDiv or errorDiv

Done.

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
remoting/client/extension/client.js:167:
errormsgDiv.appendChild(document.createTextNode(
On 2011/02/15 22:06:43, sergeyu wrote:
> I would write errormsgDiv.innerHTML = "...". It is much easier to read.

But it would be inconsistent with the style used throughout this file and we do
other appendChild calls onto this same node.

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/ma...
File remoting/client/extension/main.css (right):

http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/ma...
remoting/client/extension/main.css:156: .gaia.le.lbl { font-family: Arial,
Helvetica, sans-serif; }
On 2011/02/15 22:06:43, sergeyu wrote:
> Do we really need this to be verbatim copy from gaia? I think it is better to
> clean this up (e.g. don't specify font in 6 different places).

Done.

[Sergey Ulanov, 2011-02-17 19:45:18]

Can you please upload updated version of this change?

On 2011/02/16 18:43:06, garykac wrote:
>
http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
> File remoting/client/extension/client.js (right):
> 
>
http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
> remoting/client/extension/client.js:71: function logout_reload(form) {
> On 2011/02/15 22:06:43, sergeyu wrote:
> > use mixedCase identifiers in JS. logoutAndReload()?
> 
> Done.
> 
>
http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
> remoting/client/extension/client.js:86: function chromodir_login() {
> On 2011/02/15 22:06:43, sergeyu wrote:
> > maybe showDirectoryLogin()?
> 
> Done.
> 
>
http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
> remoting/client/extension/client.js:164: var errormsgDiv =
> document.getElementById('errormsg_div');
> On 2011/02/15 22:06:43, sergeyu wrote:
> > errorMsgDiv or errorDiv
> 
> Done.
> 
>
http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/cl...
> remoting/client/extension/client.js:167:
> errormsgDiv.appendChild(document.createTextNode(
> On 2011/02/15 22:06:43, sergeyu wrote:
> > I would write errormsgDiv.innerHTML = "...". It is much easier to read.
> 
> But it would be inconsistent with the style used throughout this file and we
do
> other appendChild calls onto this same node.
> 
>
http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/ma...
> File remoting/client/extension/main.css (right):
> 
>
http://codereview.chromium.org/6518013/diff/6001/remoting/client/extension/ma...
> remoting/client/extension/main.css:156: .gaia.le.lbl { font-family: Arial,
> Helvetica, sans-serif; }
> On 2011/02/15 22:06:43, sergeyu wrote:
> > Do we really need this to be verbatim copy from gaia? I think it is better
to
> > clean this up (e.g. don't specify font in 6 different places).
> 
> Done.

[garykac, 2011-02-17 20:13:59]

On 2011/02/17 19:45:18, sergeyu wrote:
> Can you please upload updated version of this change?

Done. Sorry about that.

[Sergey Ulanov, 2011-02-17 20:21:48]

LGTM

[Sergey Ulanov, 2011-02-17 20:22:30]

found one problem. LGTM if it is fixed.

http://codereview.chromium.org/6518013/diff/11001/remoting/client/extension/c...
File remoting/client/extension/chromoting_tab.html (right):

http://codereview.chromium.org/6518013/diff/11001/remoting/client/extension/c...
remoting/client/extension/chromoting_tab.html:26: <span class="gaia le
lbl">Username:</span>
update class name here? and below?

[garykac, 2011-02-17 20:28:33]

On 2011/02/17 20:22:30, sergeyu wrote:
> found one problem. LGTM if it is fixed.
> 
>
http://codereview.chromium.org/6518013/diff/11001/remoting/client/extension/c...
> File remoting/client/extension/chromoting_tab.html (right):
> 
>
http://codereview.chromium.org/6518013/diff/11001/remoting/client/extension/c...
> remoting/client/extension/chromoting_tab.html:26: <span class="gaia le
> lbl">Username:</span>
> update class name here? and below?

Done.