Implement new mock for user options page per chromium-os:5028

Implement new mock for user options page per chromium-os:5028

BUG=chromium-os:5028
TEST=Verify the user options page matches mock per chromium-os:5028. Note the underlying libcros is still missing and UI shows up mock data only.

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

[arv (Not doing code reviews), 2010-07-26 23:16:27]

http://codereview.chromium.org/2868067/diff/1/13
File chrome/browser/resources/options/chromeos_accounts_options_page.css
(right):

http://codereview.chromium.org/2868067/diff/1/13#newcode26
chrome/browser/resources/options/chromeos_accounts_options_page.css:26:
margin-left: 10px;
-webkit-margin-start

http://codereview.chromium.org/2868067/diff/1/13#newcode37
chrome/browser/resources/options/chromeos_accounts_options_page.css:37:
border-style: none;
border: 0;

http://codereview.chromium.org/2868067/diff/1/13#newcode38
chrome/browser/resources/options/chromeos_accounts_options_page.css:38: float:
right;
RTL?

html[dir=rtl] .remove-user-button {
  float: left;
}

But in general, floats are hardly ever needed and make things harder.

http://codereview.chromium.org/2868067/diff/1/13#newcode49
chrome/browser/resources/options/chromeos_accounts_options_page.css:49: color :
grey;
color:

http://codereview.chromium.org/2868067/diff/1/14
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2868067/diff/1/14#newcode114
chrome/browser/resources/options/chromeos_accounts_user_list.js:114: var icon =
this.ownerDocument.createElement('img');
Can't you do the icon using background-image? Less DOM nodes makes things
faster.

http://codereview.chromium.org/2868067/diff/1/14#newcode147
chrome/browser/resources/options/chromeos_accounts_user_list.js:147:
$('userList').removeUser(this.user);
this.parentNode.removeUser

I would also consider adding a single click handler to the list and check the
target. This way you don't have to add a listener to every button. However, in
this case it might not be a big deal since I doubt there will be a lot of users.

http://codereview.chromium.org/2868067/diff/1/15
File chrome/browser/resources/options/chromeos_accounts_user_name_edit.js
(right):

http://codereview.chromium.org/2868067/diff/1/15#newcode22
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:22:
this.onfocus = function(e) {
Use addEventListener to prevent overriding someone else's listener

http://codereview.chromium.org/2868067/diff/1/15#newcode32
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:32:
this.value = localStrings.getString('username_edit_hint');
This can be done using HTML5+CSS

<style>

input + span {
  opacity: .3;
}

input:invalid + span {
  opacity: 1;
  color: red;
}

</style>
<input required> <span>required</span>

http://codereview.chromium.org/2868067/diff/1/15#newcode53
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:53: *
@param {String} str A string to parse.
{string}

String is the object wrapper of String and should never be used

http://codereview.chromium.org/2868067/diff/1/15#newcode60
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:60: const
Format1 = /^([\w\.!#\$%&'\*\+-\/=\?\^`\{\|\}~]+)$/;
Only class names and enums should be capitalized

http://codereview.chromium.org/2868067/diff/1/15#newcode106
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:106: var
str = this.value.replace(/^\s+|\s+$/g, '');
Take a look at HTML5 form validation.

<style>

input + span {
  opacity: .3;
}

input:invalid + span {
  opacity: 1;
  color: red;
}

</style>
<input pattern=".+@.+\..+" required> <span>required</span>

I'm not sure this changes anything but it seems we could provide good feedback
here.

[xiyuan, 2010-07-27 19:12:47]

CL updated, major changes:
- Use HTML5 input placeholder attribute instead of using onfocus/onblur event
handler for user name edit hint/instruction text;
- Use HTML5 form validation as suggested;

The only thing that I did not change is still using <img> for user icon. This is
because I don't know how to add the black border if using background image.

http://codereview.chromium.org/2868067/diff/1/13
File chrome/browser/resources/options/chromeos_accounts_options_page.css
(right):

http://codereview.chromium.org/2868067/diff/1/13#newcode26
chrome/browser/resources/options/chromeos_accounts_options_page.css:26:
margin-left: 10px;
On 2010/07/26 23:16:27, arv wrote:
> -webkit-margin-start

Done.

http://codereview.chromium.org/2868067/diff/1/13#newcode37
chrome/browser/resources/options/chromeos_accounts_options_page.css:37:
border-style: none;
On 2010/07/26 23:16:27, arv wrote:
> border: 0;

Done.

http://codereview.chromium.org/2868067/diff/1/13#newcode38
chrome/browser/resources/options/chromeos_accounts_options_page.css:38: float:
right;
On 2010/07/26 23:16:27, arv wrote:
> RTL?
> 
> html[dir=rtl] .remove-user-button {
>   float: left;
> }
> 
> But in general, floats are hardly ever needed and make things harder.

Done.

My question would then be what would be the preferred to way to do this (having
an element right aligned to its container element)?

http://codereview.chromium.org/2868067/diff/1/13#newcode49
chrome/browser/resources/options/chromeos_accounts_options_page.css:49: color :
grey;
On 2010/07/26 23:16:27, arv wrote:
> color:

Done.

http://codereview.chromium.org/2868067/diff/1/14
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2868067/diff/1/14#newcode114
chrome/browser/resources/options/chromeos_accounts_user_list.js:114: var icon =
this.ownerDocument.createElement('img');
On 2010/07/26 23:16:27, arv wrote:
> Can't you do the icon using background-image? Less DOM nodes makes things
> faster.

The mock has a 1px solid black border around the user icon and not sure if we
could do that using background image.

http://codereview.chromium.org/2868067/diff/1/14#newcode147
chrome/browser/resources/options/chromeos_accounts_user_list.js:147:
$('userList').removeUser(this.user);
On 2010/07/26 23:16:27, arv wrote:
> this.parentNode.removeUser
> 
> I would also consider adding a single click handler to the list and check the
> target. This way you don't have to add a listener to every button. However, in
> this case it might not be a big deal since I doubt there will be a lot of
users.

Done. Changed to use list click handler as suggested.

http://codereview.chromium.org/2868067/diff/1/15
File chrome/browser/resources/options/chromeos_accounts_user_name_edit.js
(right):

http://codereview.chromium.org/2868067/diff/1/15#newcode22
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:22:
this.onfocus = function(e) {
On 2010/07/26 23:16:27, arv wrote:
> Use addEventListener to prevent overriding someone else's listener

Using HTML5 placeholder attribute to implement the hint now and removed the
onfocus and onblur handler.

http://codereview.chromium.org/2868067/diff/1/15#newcode32
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:32:
this.value = localStrings.getString('username_edit_hint');
On 2010/07/26 23:16:27, arv wrote:
> This can be done using HTML5+CSS
> 
> <style>
> 
> input + span {
>   opacity: .3;
> }
> 
> input:invalid + span {
>   opacity: 1;
>   color: red;
> }
> 
> </style>
> <input required> <span>required</span>

onblur function is removed as new HTML5 placeholder attribute makes it obsolete.
But your point is taken and switched to use HTML5 form validation.

http://codereview.chromium.org/2868067/diff/1/15#newcode53
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:53: *
@param {String} str A string to parse.
On 2010/07/26 23:16:27, arv wrote:
> {string}
> 
> String is the object wrapper of String and should never be used

Done.

http://codereview.chromium.org/2868067/diff/1/15#newcode60
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:60: const
Format1 = /^([\w\.!#\$%&'\*\+-\/=\?\^`\{\|\}~]+)$/;
On 2010/07/26 23:16:27, arv wrote:
> Only class names and enums should be capitalized

Done.

http://codereview.chromium.org/2868067/diff/1/15#newcode106
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:106: var
str = this.value.replace(/^\s+|\s+$/g, '');
On 2010/07/26 23:16:27, arv wrote:
> Take a look at HTML5 form validation.
> 
> <style>
> 
> input + span {
>   opacity: .3;
> }
> 
> input:invalid + span {
>   opacity: 1;
>   color: red;
> }
> 
> </style>
> <input pattern=".+@.+\..+" required> <span>required</span>
> 
> I'm not sure this changes anything but it seems we could provide good feedback
> here.

Done. Switched to use HTML5 form validation per suggested.

[arv (Not doing code reviews), 2010-07-27 21:38:46]

http://codereview.chromium.org/2868067/diff/15001/16012
File chrome/browser/resources/options/chromeos_accounts_options_page.css
(right):

http://codereview.chromium.org/2868067/diff/15001/16012#newcode61
chrome/browser/resources/options/chromeos_accounts_options_page.css:61: display:
block;
I'm afraid this will make the UI move around. Can you use visibility and/or
opacity+pointer-events?

http://codereview.chromium.org/2868067/diff/15001/16013
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2868067/diff/15001/16013#newcode79
chrome/browser/resources/options/chromeos_accounts_user_list.js:79: if
(el.tagName == 'BUTTON' && el.className == 'remove-user-button') {
It seems like the check for tagName can be skipped

http://codereview.chromium.org/2868067/diff/15001/16014
File chrome/browser/resources/options/chromeos_accounts_user_name_edit.js
(right):

http://codereview.chromium.org/2868067/diff/15001/16014#newcode101
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:101: if
(e.keyCode == 13) {
Add comment  // Enter

http://codereview.chromium.org/2868067/diff/15001/16014#newcode104
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:104:
$('userList').addUser(user);
This seems like a breach of abstraction.

Maybe you could use an event or a property that fires property change events.

[xiyuan, 2010-07-27 22:23:56]

http://codereview.chromium.org/2868067/diff/15001/16012
File chrome/browser/resources/options/chromeos_accounts_options_page.css
(right):

http://codereview.chromium.org/2868067/diff/15001/16012#newcode61
chrome/browser/resources/options/chromeos_accounts_options_page.css:61: display:
block;
On 2010/07/27 21:38:46, arv wrote:
> I'm afraid this will make the UI move around. Can you use visibility and/or
> opacity+pointer-events?

How about make the input background red to indicate error rather than having a
text?

The mock screen is attached in http://crosbug.com/5028. It does not look nice
with visibility hidden. :(

http://codereview.chromium.org/2868067/diff/15001/16013
File chrome/browser/resources/options/chromeos_accounts_user_list.js (right):

http://codereview.chromium.org/2868067/diff/15001/16013#newcode79
chrome/browser/resources/options/chromeos_accounts_user_list.js:79: if
(el.tagName == 'BUTTON' && el.className == 'remove-user-button') {
On 2010/07/27 21:38:46, arv wrote:
> It seems like the check for tagName can be skipped

Done.

http://codereview.chromium.org/2868067/diff/15001/16014
File chrome/browser/resources/options/chromeos_accounts_user_name_edit.js
(right):

http://codereview.chromium.org/2868067/diff/15001/16014#newcode101
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:101: if
(e.keyCode == 13) {
On 2010/07/27 21:38:46, arv wrote:
> Add comment  // Enter

Done.

http://codereview.chromium.org/2868067/diff/15001/16014#newcode104
chrome/browser/resources/options/chromeos_accounts_user_name_edit.js:104:
$('userList').addUser(user);
On 2010/07/27 21:38:46, arv wrote:
> This seems like a breach of abstraction.
> 
> Maybe you could use an event or a property that fires property change events.

Done.

[arv (Not doing code reviews), 2010-07-28 06:13:25]

LGTM