Update thumbnail/list view buttons.

Update thumbnail/list view buttons.

BUG=chromium-os:20161
TEST=Ctrl+m or ctrl+o or ctrl+s to open file manager/dialog, verify the view
buttons is the same as in http://folder/kenmoore/mocks/chromeos/Misc_2012/FileBrowser/VisualMaster_01-03-2012/


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

[bshe, 2012-01-10 21:21:22]

Hey Rob.

Could you please take a look at this CL? It updates the file manager view
selection buttons assets.

Thanks!

Biao

[flackr, 2012-01-10 22:02:55]

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:2: * Copyright (c)
2011 The Chromium Authors. All rights reserved.
s/2011/2012

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:334:
button.thumbnail-view {
If you can remove the outer div, change this to .dialog-header > button {

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:339: min-width: 0;
Did you mean to specify this as well as width below?

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:345:
button.detail-view:hover {
Seems like you shouldn't need a selector for the hover states as well. Can you
remove these?

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:3: -- Copyright (c) 2011 The
Chromium Authors. All rights reserved.
s/2011/2012

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:145: <div><button
class=detail-view tabindex=4 disabled></button></div>
Get rid of the surrounding div if it is not being used for anything.

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:146: <div><button
class=thumbnail-view tabindex=5></button></div>
All of the styles in file_manager are wrong (I.e. attribute values should be
quoted with double quotes in html files, css belongs in css files, scripts
belong in js files.) I hate to see more code go in preserving the bad style.
Perhaps you could file a bug to track cleaning up the code in this directory.

[bshe, 2012-01-11 01:16:21]

Thanks!

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:2: * Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/01/10 22:02:55, flackr wrote:
> s/2011/2012

Done.

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:334:
button.thumbnail-view {
On 2012/01/10 22:02:55, flackr wrote:
> If you can remove the outer div, change this to .dialog-header > button {

outer div is a hack for extra margin between buttons. Use .dialog-header > div >
button instead.

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:339: min-width: 0;
On 2012/01/10 22:02:55, flackr wrote:
> Did you mean to specify this as well as width below?

Yes. I need it to override this:
button,
input[type='button'],
input[type='submit'] {
  min-width: 4em;
}
It seems 28px is smaller than 4em in this case.

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:345:
button.detail-view:hover {
On 2012/01/10 22:02:55, flackr wrote:
> Seems like you shouldn't need a selector for the hover states as well. Can you
> remove these?

The hover states aren't necessary only when these background-img rules appears
after these rules in css file:
button:hover {
  background: #ebebeb -webkit-linear-gradient(#fefefe, #f8f8f8 40%, #e9e9e9);
}
and
button[disabled]:hover {
  background: -webkit-linear-gradient(#fafafa, #f4f4f4 40%, #e5e5e5);
}

I can either add a comment here to say do not change the order of these rules or
keep the hover state here just in case someone move these rules and causing
problem. Which way do you suggest?

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:3: -- Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/01/10 22:02:55, flackr wrote:
> s/2011/2012

Done.

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:145: <div><button
class=detail-view tabindex=4 disabled></button></div>
Sorry. I should add a comment here. The surrounding div is a hack to get rid of
the margin between the two buttons. Matt mentioned that you and him have seen
the margin problem before.

On 2012/01/10 22:02:55, flackr wrote:
> Get rid of the surrounding div if it is not being used for anything.

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:146: <div><button
class=thumbnail-view tabindex=5></button></div>
On 2012/01/10 22:02:55, flackr wrote:
> All of the styles in file_manager are wrong (I.e. attribute values should be
> quoted with double quotes in html files, css belongs in css files, scripts
> belong in js files.) I hate to see more code go in preserving the bad style.
> Perhaps you could file a bug to track cleaning up the code in this directory.

Done. Tracked in chromium-os:24926

[flackr, 2012-01-11 16:25:45]

lgtm, you will probably need an owner though.

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

http://codereview.chromium.org/9114040/diff/1001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/css/file_manager.css:345:
button.detail-view:hover {
On 2012/01/11 01:16:21, bshe wrote:
> On 2012/01/10 22:02:55, flackr wrote:
> > Seems like you shouldn't need a selector for the hover states as well. Can
you
> > remove these?
> 
> The hover states aren't necessary only when these background-img rules appears
> after these rules in css file:
> button:hover {
>   background: #ebebeb -webkit-linear-gradient(#fefefe, #f8f8f8 40%, #e9e9e9);
> }
> and
> button[disabled]:hover {
>   background: -webkit-linear-gradient(#fafafa, #f4f4f4 40%, #e5e5e5);
> }
> 
> I can either add a comment here to say do not change the order of these rules
or
> keep the hover state here just in case someone move these rules and causing
> problem. Which way do you suggest?

I suggest a comment to the effect that you must override the defined
button:hover and button[disabled]:hover rules.

[rginda, 2012-01-11 16:43:59]

Lgtm

[bshe, 2012-01-11 18:21:59]

+OWNERS for
chrome/browser/resources/component_extension_resources.grd

Thanks!

[James Hawkins, 2012-01-11 18:23:55]

lgtm

[commit-bot: I haz the power, 2012-01-11 18:27:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/9114040/9003

[commit-bot: I haz the power, 2012-01-11 21:27:38]

Change committed as 117283