Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
396193002:
Fix avatar image size in user pods on desktop (Closed)
|
Created:
6 years, 5 months ago by bartfab (slow) Modified:
6 years, 5 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix avatar image size in user pods on desktop
This is a fix for a regression introduced by CL 358183004. On desktop,
small avatar images should not be centered but stretched to fill the
entire available space of 180px x 180px.
BUG=214904, 241790
TEST=Manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285683
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fixed missing semicolon. Alphabetized. #Patch Set 3 : Addressed comments. #Patch Set 4 : Fix opacity of focused pods. #
Total comments: 2
Patch Set 5 : Added TODO. #
Messages
Total messages: 26 (0 generated)
Hi Monica, Could you take a look please? Hi Evan, Could you do an owner review please?
This looks like it works. Thanks!
lgtm On 2014/07/16 13:51:13, Monica Dinculescu wrote: > This looks like it works. Thanks!
https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... File chrome/browser/resources/user_manager/user_manager.css (right): https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... chrome/browser/resources/user_manager/user_manager.css:63: max-width: initial; is there some other rule setting this somewhere? why can't you just leave it off? https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... chrome/browser/resources/user_manager/user_manager.css:64: width: 180px ;
https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... File chrome/browser/resources/user_manager/user_manager.css (right): https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... chrome/browser/resources/user_manager/user_manager.css:63: max-width: initial; On 2014/07/16 23:26:24, Evan Stade wrote: > is there some other rule setting this somewhere? why can't you just leave it > off? It's inherited from the CSS for Chrome OS pods, which the desktop pods are based on: https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/log... (lines 86-87)
https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... File chrome/browser/resources/user_manager/user_manager.css (right): https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... chrome/browser/resources/user_manager/user_manager.css:63: max-width: initial; On 2014/07/16 23:53:57, bartfab wrote: > On 2014/07/16 23:26:24, Evan Stade wrote: > > is there some other rule setting this somewhere? why can't you just leave it > > off? > > It's inherited from the CSS for Chrome OS pods, which the desktop pods are based > on: > > https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/log... > (lines 86-87) I think it's a little dangerous to include css from ../login because it's not an obviously shared location. What happens when someone makes a change to chromeos login and it has an unexpected affect on the user manager? I'd rather the shared CSS be moved to an obviously shared location, and the max-height/max-width stuff in user_pod_row.css kept in an unshared css file.
On 2014/07/17 01:00:08, Evan Stade wrote: > https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... > File chrome/browser/resources/user_manager/user_manager.css (right): > > https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... > chrome/browser/resources/user_manager/user_manager.css:63: max-width: initial; > On 2014/07/16 23:53:57, bartfab wrote: > > On 2014/07/16 23:26:24, Evan Stade wrote: > > > is there some other rule setting this somewhere? why can't you just leave it > > > off? > > > > It's inherited from the CSS for Chrome OS pods, which the desktop pods are > based > > on: > > > > > https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/log... > > (lines 86-87) > > I think it's a little dangerous to include css from ../login because it's not an > obviously shared location. What happens when someone makes a change to chromeos > login and it has an unexpected affect on the user manager? > > I'd rather the shared CSS be moved to an obviously shared location, and the > max-height/max-width stuff in user_pod_row.css kept in an unshared css file. This is a valid concern but I do not think it should be solved in the current CL. This CL just updates an existing CSS file. Moving CSS files to a different location should be tracked separately. FWIW, Monica is the author of user_manager.css, so she is probably best qualified to rearrange files in the best way.
On 2014/07/21 14:40:11, bartfab wrote: > On 2014/07/17 01:00:08, Evan Stade wrote: > > > https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... > > File chrome/browser/resources/user_manager/user_manager.css (right): > > > > > https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... > > chrome/browser/resources/user_manager/user_manager.css:63: max-width: initial; > > On 2014/07/16 23:53:57, bartfab wrote: > > > On 2014/07/16 23:26:24, Evan Stade wrote: > > > > is there some other rule setting this somewhere? why can't you just leave > it > > > > off? > > > > > > It's inherited from the CSS for Chrome OS pods, which the desktop pods are > > based > > > on: > > > > > > > > > https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/log... > > > (lines 86-87) > > > > I think it's a little dangerous to include css from ../login because it's not > an > > obviously shared location. What happens when someone makes a change to > chromeos > > login and it has an unexpected affect on the user manager? > > > > I'd rather the shared CSS be moved to an obviously shared location, and the > > max-height/max-width stuff in user_pod_row.css kept in an unshared css file. > > This is a valid concern but I do not think it should be solved in the current > CL. This CL just updates an existing CSS file. Moving CSS files to a different > location should be tracked separately. I don't think rearranging the CSS would actually take very long, but if you don't want to tackle it in this CL then the best temporary fix would be to edit ../login/user_pod_row.css to only apply the max-width and max-height for pods on the login page. That has the benefit of making it more obvious what to do when someone gets around to refactoring, and also sticking out like a sore thumb (which will encourage the refactor to come sooner rather than later). > > FWIW, Monica is the author of user_manager.css, so she is probably best > qualified to rearrange files in the best way.
Monica, Pavel, You tell me how you want to handle this: - user_pod_row.css contains *no* desktop-specific rules. - user_manager.css contains *only* desktop-specific rules. Are you OK with me sticking a desktop-specific rule into user_pod_row.css? Do you want to refactor the code layout as suggested by Evan? Do you prefer a different solution altogether? I am not the owner or maintainer of any of these files, so I do not want to be one making this decision.
https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... File chrome/browser/resources/user_manager/user_manager.css (right): https://codereview.chromium.org/396193002/diff/1/chrome/browser/resources/use... chrome/browser/resources/user_manager/user_manager.css:64: width: 180px On 2014/07/16 23:26:24, Evan Stade wrote: > ; Done.
Monica, Pavel, ping.
Oof, I would lean towards keeping the desktop-specific rules in user_pod_row.css, as it is now. I think Nikita already has a CL started about componetizing this code (https://codereview.chromium.org/402403005/), so he might already be working on a related refactor. Evan: we actually moved all the desktop/chromeos common code to /login (it used to be in chromeos/login). So I think the expectation is that if anyone makes a change to /login, it should affect both chromeos and desktop. On 2014/07/23 08:34:19, bartfab wrote: > Monica, Pavel, ping.
> I think it's a little dangerous to include css from ../login because it's not an > obviously shared location. All the developers that work with '../login' code are aware that this code is shared, so it is not a problem. The problem is that 'user_pod_row.css' is not a shared part of CSS, but it is an CSS for CrOS login screen, and desktop user manager just overrides some rules. Probably the proper solution is to split 'user_pod_row.css' into two parts, the first of which is shared between desktop and CrOS and the second contains only rules specific to CrOS.
On 2014/07/24 14:47:25, dzhioev wrote: > > I think it's a little dangerous to include css from ../login because it's not > an > > obviously shared location. > All the developers that work with '../login' code are aware that this code is > shared, > so it is not a problem. The problem is that 'user_pod_row.css' is not a shared > part > of CSS, but it is an CSS for CrOS login screen, and desktop user manager just > overrides > some rules. Probably the proper solution is to split 'user_pod_row.css' into two > parts, > the first of which is shared between desktop and CrOS and the second contains > only rules > specific to CrOS. I believe this split is what Evan is talking about. But that does not help us in the short term: If we want to fix the avatar size, we should land this CL as-is, then look into refactoring the CSS.
On 2014/07/24 14:55:39, bartfab wrote:
> On 2014/07/24 14:47:25, dzhioev wrote:
> > > I think it's a little dangerous to include css from ../login because it's
> not
> > an
> > > obviously shared location.
> > All the developers that work with '../login' code are aware that this code
is
> > shared,
> > so it is not a problem. The problem is that 'user_pod_row.css' is not a
shared
> > part
> > of CSS, but it is an CSS for CrOS login screen, and desktop user manager
just
> > overrides
> > some rules. Probably the proper solution is to split 'user_pod_row.css' into
> two
> > parts,
> > the first of which is shared between desktop and CrOS and the second
contains
> > only rules
> > specific to CrOS.
>
> I believe this split is what Evan is talking about. But that does not help us
in
> the short term: If we want to fix the avatar size, we should land this CL
as-is,
> then look into refactoring the CSS.
again, why not qualify the max width/height rules in user_pod_row.css with
something like #login-page .pod { ... }? Then the later refactor is more
apparent instead of super subtle.
On 2014/07/24 15:01:39, Evan Stade wrote:
> On 2014/07/24 14:55:39, bartfab wrote:
> > On 2014/07/24 14:47:25, dzhioev wrote:
> > > > I think it's a little dangerous to include css from ../login because
it's
> > not
> > > an
> > > > obviously shared location.
> > > All the developers that work with '../login' code are aware that this code
> is
> > > shared,
> > > so it is not a problem. The problem is that 'user_pod_row.css' is not a
> shared
> > > part
> > > of CSS, but it is an CSS for CrOS login screen, and desktop user manager
> just
> > > overrides
> > > some rules. Probably the proper solution is to split 'user_pod_row.css'
into
> > two
> > > parts,
> > > the first of which is shared between desktop and CrOS and the second
> contains
> > > only rules
> > > specific to CrOS.
> >
> > I believe this split is what Evan is talking about. But that does not help
us
> in
> > the short term: If we want to fix the avatar size, we should land this CL
> as-is,
> > then look into refactoring the CSS.
>
> again, why not qualify the max width/height rules in user_pod_row.css with
> something like #login-page .pod { ... }? Then the later refactor is more
> apparent instead of super subtle.
Because the idea is to keep any desktop-specific CSS out of user_pod_row.css. We
have no precedent polluting it with desktop CSS rules.
On 2014/07/24 15:03:59, bartfab wrote:
> On 2014/07/24 15:01:39, Evan Stade wrote:
> > On 2014/07/24 14:55:39, bartfab wrote:
> > > On 2014/07/24 14:47:25, dzhioev wrote:
> > > > > I think it's a little dangerous to include css from ../login because
> it's
> > > not
> > > > an
> > > > > obviously shared location.
> > > > All the developers that work with '../login' code are aware that this
code
> > is
> > > > shared,
> > > > so it is not a problem. The problem is that 'user_pod_row.css' is not a
> > shared
> > > > part
> > > > of CSS, but it is an CSS for CrOS login screen, and desktop user manager
> > just
> > > > overrides
> > > > some rules. Probably the proper solution is to split 'user_pod_row.css'
> into
> > > two
> > > > parts,
> > > > the first of which is shared between desktop and CrOS and the second
> > contains
> > > > only rules
> > > > specific to CrOS.
> > >
> > > I believe this split is what Evan is talking about. But that does not help
> us
> > in
> > > the short term: If we want to fix the avatar size, we should land this CL
> > as-is,
> > > then look into refactoring the CSS.
> >
> > again, why not qualify the max width/height rules in user_pod_row.css with
> > something like #login-page .pod { ... }? Then the later refactor is more
> > apparent instead of super subtle.
>
> Because the idea is to keep any desktop-specific CSS out of user_pod_row.css.
We
> have no precedent polluting it with desktop CSS rules.
a) that's not a desktop rule.
b) I agree that is ugly. You could refactor the files instead. But this patch is
even uglier, because it's so subtle.
On 2014/07/24 15:24:59, Evan Stade wrote:
> On 2014/07/24 15:03:59, bartfab wrote:
> > On 2014/07/24 15:01:39, Evan Stade wrote:
> > > On 2014/07/24 14:55:39, bartfab wrote:
> > > > On 2014/07/24 14:47:25, dzhioev wrote:
> > > > > > I think it's a little dangerous to include css from ../login because
> > it's
> > > > not
> > > > > an
> > > > > > obviously shared location.
> > > > > All the developers that work with '../login' code are aware that this
> code
> > > is
> > > > > shared,
> > > > > so it is not a problem. The problem is that 'user_pod_row.css' is not
a
> > > shared
> > > > > part
> > > > > of CSS, but it is an CSS for CrOS login screen, and desktop user
manager
> > > just
> > > > > overrides
> > > > > some rules. Probably the proper solution is to split
'user_pod_row.css'
> > into
> > > > two
> > > > > parts,
> > > > > the first of which is shared between desktop and CrOS and the second
> > > contains
> > > > > only rules
> > > > > specific to CrOS.
> > > >
> > > > I believe this split is what Evan is talking about. But that does not
help
> > us
> > > in
> > > > the short term: If we want to fix the avatar size, we should land this
CL
> > > as-is,
> > > > then look into refactoring the CSS.
> > >
> > > again, why not qualify the max width/height rules in user_pod_row.css with
> > > something like #login-page .pod { ... }? Then the later refactor is more
> > > apparent instead of super subtle.
> >
> > Because the idea is to keep any desktop-specific CSS out of
user_pod_row.css.
> We
> > have no precedent polluting it with desktop CSS rules.
>
> a) that's not a desktop rule.
> b) I agree that is ugly. You could refactor the files instead. But this patch
is
> even uglier, because it's so subtle.
I agree that Evan's proposal is better then current patch.
The selector you are needed is "html:not([screen=login-add-user]) .pod
.user-image".
I believe this implementation matches the consensus we reached. Please take another look.
On 2014/07/25 11:51:46, bartfab wrote: > I believe this implementation matches the consensus we reached. Please take > another look. LGTM
lgtm https://codereview.chromium.org/396193002/diff/60001/ui/login/account_picker/... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/396193002/diff/60001/ui/login/account_picker/... ui/login/account_picker/user_pod_row.css:88: html:not([screen=login-add-user]) .pod .user-image { could you add a TODO/file a bug for this?
https://codereview.chromium.org/396193002/diff/60001/ui/login/account_picker/... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/396193002/diff/60001/ui/login/account_picker/... ui/login/account_picker/user_pod_row.css:88: html:not([screen=login-add-user]) .pod .user-image { On 2014/07/25 18:47:50, Evan Stade wrote: > could you add a TODO/file a bug for this? Done.
The CQ bit was checked by bartfab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/396193002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
Message was sent while issue was closed.
Change committed as 285683 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
