Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2498293003:
CrOS md user row adjustments (Closed)
|
Created:
4 years, 1 month ago by Evan Stade Modified:
4 years, 1 month ago Reviewers:
tdanderson CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCrOS md user row adjustments
1) make height of signed-in user row correct (extra padding above and
below)
2) get rid of bubble-on-bubble for add user error case. Replace the row
that says "Add new user" with error text instead. Also fix add user
row size.
3) fix color of separator
4) fix width of sign out button
5) fix fonts of button, user name and user email
6) fix color and alpha of not-active-user rows (no grayscale avatar,
whole row is 50% alpha)
This does NOT fix non-active-user rows showing show the user name,
(they don't currently but they should) however that will somehow
have to deal with the media indicator, so I'm punting.
BUG=632153, 665987, 658818, 658825
Committed: https://crrev.com/87842b52fc6b0a6343afeffc8a07307849d1e98c
Cr-Commit-Position: refs/heads/master@{#432730}
Patch Set 1 #Patch Set 2 : oops #Patch Set 3 : simpler slightly #Patch Set 4 : more stuff #
Total comments: 15
Patch Set 5 : is_active_user() #
Messages
Total messages: 30 (22 generated)
Description was changed from ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator BUG=632153 ========== to ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator BUG=632153 ==========
estade@chromium.org changed reviewers: + tdanderson@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator BUG=632153 ========== to ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale Avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153 ==========
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale Avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153 ========== to ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/rounded_image_view.h (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/rounded_image_view.h:47: // TODO(estade): remove this, it's not used in Material Design. nit: please add crbug.com/614453 https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_card_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_card_view.cc:379: if (user_index != 0) { Consider sharing UserView::IsActiveUser() logic, perhaps moving that to a common place / making static. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_card_view.cc:489: void UserCardView::AddUserContentMd(LoginStatus login_status) { It seems the similarity between this and the non-MD counterpart is large enough that it might make more sense to keep within one function and just add if(use_md) {...} else {...} in ~4-5 places. I'll leave that up to you though. (If you leave as is, it might be worthwhile including a note in the header that the MD version of this function is basically a fork of the non-MD version). https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_view.cc:115: error ? 56 : GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)); nit: can you derive this 56 from other constants or give it a name?
https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/rounded_image_view.h (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/rounded_image_view.h:47: // TODO(estade): remove this, it's not used in Material Design. On 2016/11/16 23:08:08, tdanderson wrote: > nit: please add crbug.com/614453 so the reason I don't add that religiously is because I have a pretty good grasp of how to remove pre-md stuff --- I had a whole lot of practice on top Chrome --- and as long as the word "material" is somewhere nearby, it won't be hard to grep for. On the other hand, if all you ever grep for is that bug number, you'll miss a whole lot of stuff that should be removed. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_card_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_card_view.cc:379: if (user_index != 0) { On 2016/11/16 23:08:08, tdanderson wrote: > Consider sharing UserView::IsActiveUser() logic, perhaps moving that to a common > place / making static. I don't like the idea of sharing such simple logic (and adding verbosity at callsites), but I added is_active_user() to UserCardView. It's used in precisely two places. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_card_view.cc:489: void UserCardView::AddUserContentMd(LoginStatus login_status) { On 2016/11/16 23:08:08, tdanderson wrote: > It seems the similarity between this and the non-MD counterpart is large enough > that it might make more sense to keep within one function and just add > if(use_md) {...} else {...} in ~4-5 places. I'll leave that up to you though. > (If you leave as is, it might be worthwhile including a note in the header that > the MD version of this function is basically a fork of the non-MD version). It's way harder to sprinkle use_md around and not break anything and to have the logic be easy to follow. It's way easier to untangle MD and non-MD when the time comes to do so if you have forked. Regarding the note in the header --- this uses a very common naming convention for forked/alternate code and I have a hard time understanding how someone could fail to notice this when it comes time to rip out pre-md code since AddUserContent would become just a call-through to AddUserContentMd. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_view.cc:115: error ? 56 : GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)); On 2016/11/16 23:08:08, tdanderson wrote: > nit: can you derive this 56 from other constants or give it a name? part 1: I don't think so, it's basically just a magic number Sebastien came up with AFAICT. part 2: If you mean const int kMinimumHeightForWhenTheresAnError = 56; layout->set_minimum_cross_axis_size( error ? kMinimumHeightForWhenTheresAnError : GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)); I don't think it adds legibility. Do you? Or did you have something else in mind?
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/rounded_image_view.h (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/rounded_image_view.h:47: // TODO(estade): remove this, it's not used in Material Design. On 2016/11/17 00:37:50, Evan Stade wrote: > On 2016/11/16 23:08:08, tdanderson wrote: > > nit: please add crbug.com/614453 > > so the reason I don't add that religiously is because I have a pretty good grasp > of how to remove pre-md stuff --- I had a whole lot of practice on top Chrome > --- and as long as the word "material" is somewhere nearby, it won't be hard to > grep for. > > On the other hand, if all you ever grep for is that bug number, you'll miss a > whole lot of stuff that should be removed. The plan is to grep for a) places that are gated with something in MaterialDesignController, and b) the bug number. If this is something that is in your opinion easily discovered with a) (even if you are not necessarily the one who will be doing the cleanup in this file) then please disregard (now and in all future times I may make the same suggestion). https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_card_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_card_view.cc:379: if (user_index != 0) { On 2016/11/17 00:37:50, Evan Stade wrote: > On 2016/11/16 23:08:08, tdanderson wrote: > > Consider sharing UserView::IsActiveUser() logic, perhaps moving that to a > common > > place / making static. > > I don't like the idea of sharing such simple logic (and adding verbosity at > callsites), but I added is_active_user() to UserCardView. It's used in precisely > two places. Thanks. One person's verbosity is perhaps another person's readability. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_card_view.cc:489: void UserCardView::AddUserContentMd(LoginStatus login_status) { On 2016/11/17 00:37:50, Evan Stade wrote: > On 2016/11/16 23:08:08, tdanderson wrote: > > It seems the similarity between this and the non-MD counterpart is large > enough > > that it might make more sense to keep within one function and just add > > if(use_md) {...} else {...} in ~4-5 places. I'll leave that up to you though. > > (If you leave as is, it might be worthwhile including a note in the header > that > > the MD version of this function is basically a fork of the non-MD version). > > It's way harder to sprinkle use_md around and not break anything and to have the > logic be easy to follow. It's way easier to untangle MD and non-MD when the time > comes to do so if you have forked. > > Regarding the note in the header --- this uses a very common naming convention > for forked/alternate code and I have a hard time understanding how someone could > fail to notice this when it comes time to rip out pre-md code since > AddUserContent would become just a call-through to AddUserContentMd. This all sounds very reasonable, and as I said, I leave it up to you with regard to the style you decide (fork vs. inline is_md checks). By forking you theoretically run the risk of the implementations drifting, but in reality that shouldn't happen since you're the only one touching this code and we are close to beta branch. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_view.cc:115: error ? 56 : GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)); On 2016/11/17 00:37:50, Evan Stade wrote: > On 2016/11/16 23:08:08, tdanderson wrote: > > nit: can you derive this 56 from other constants or give it a name? > > part 1: I don't think so, it's basically just a magic number Sebastien came up > with AFAICT. > > part 2: If you mean > > const int kMinimumHeightForWhenTheresAnError = 56; > layout->set_minimum_cross_axis_size( > error ? kMinimumHeightForWhenTheresAnError : > GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)); > > I don't think it adds legibility. Do you? Or did you have something else in > mind? If it is truly just a magic number that cannot be given a meaningful name, then I agree my suggestion will be counterproductive. My original suggestion should have read "... or give it a meaningful name".
Description was changed from ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153 ========== to ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153,665987,658818 ==========
Description was changed from ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153,665987,658818 ========== to ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153,665987,658818,658825 ==========
https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/rounded_image_view.h (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/rounded_image_view.h:47: // TODO(estade): remove this, it's not used in Material Design. On 2016/11/17 01:40:15, tdanderson wrote: > On 2016/11/17 00:37:50, Evan Stade wrote: > > On 2016/11/16 23:08:08, tdanderson wrote: > > > nit: please add crbug.com/614453 > > > > so the reason I don't add that religiously is because I have a pretty good > grasp > > of how to remove pre-md stuff --- I had a whole lot of practice on top Chrome > > --- and as long as the word "material" is somewhere nearby, it won't be hard > to > > grep for. > > > > On the other hand, if all you ever grep for is that bug number, you'll miss a > > whole lot of stuff that should be removed. > > The plan is to grep for a) places that are gated with something in > MaterialDesignController, and b) the bug number. If this is something that is in > your opinion easily discovered with a) (even if you are not necessarily the one > who will be doing the cleanup in this file) then please disregard (now and in > all future times I may make the same suggestion). one reason a) and b) are not sufficient alone is because as you change code you need to change a lot of comments as well. Many comments reference differences between MD and non-MD, but do not reference that bug, and the compiler would not complain if you removed IsShelfMaterial() but didn't touch the comments. Thus I think the best approach is grepping for "material" and "md" (which covers comments, function names like DoFooMd, and MaterialDesignController::IsFooMaterial, and has very few false positives), and making sure that grepping for those two things is sufficient. At that point, the bug number is superfluous. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_card_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_card_view.cc:489: void UserCardView::AddUserContentMd(LoginStatus login_status) { On 2016/11/17 01:40:15, tdanderson wrote: > On 2016/11/17 00:37:50, Evan Stade wrote: > > On 2016/11/16 23:08:08, tdanderson wrote: > > > It seems the similarity between this and the non-MD counterpart is large > > enough > > > that it might make more sense to keep within one function and just add > > > if(use_md) {...} else {...} in ~4-5 places. I'll leave that up to you > though. > > > (If you leave as is, it might be worthwhile including a note in the header > > that > > > the MD version of this function is basically a fork of the non-MD version). > > > > It's way harder to sprinkle use_md around and not break anything and to have > the > > logic be easy to follow. It's way easier to untangle MD and non-MD when the > time > > comes to do so if you have forked. > > > > Regarding the note in the header --- this uses a very common naming convention > > for forked/alternate code and I have a hard time understanding how someone > could > > fail to notice this when it comes time to rip out pre-md code since > > AddUserContent would become just a call-through to AddUserContentMd. > > This all sounds very reasonable, and as I said, I leave it up to you with regard > to the style you decide (fork vs. inline is_md checks). By forking you > theoretically run the risk of the implementations drifting, but in reality that > shouldn't happen since you're the only one touching this code and we are close > to beta branch. indeed, that is another consideration that I might take into account if we were several milestones out. https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... File ash/common/system/user/user_view.cc (right): https://codereview.chromium.org/2498293003/diff/60001/ash/common/system/user/... ash/common/system/user/user_view.cc:115: error ? 56 : GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)); On 2016/11/17 01:40:15, tdanderson wrote: > On 2016/11/17 00:37:50, Evan Stade wrote: > > On 2016/11/16 23:08:08, tdanderson wrote: > > > nit: can you derive this 56 from other constants or give it a name? > > > > part 1: I don't think so, it's basically just a magic number Sebastien came up > > with AFAICT. > > > > part 2: If you mean > > > > const int kMinimumHeightForWhenTheresAnError = 56; > > layout->set_minimum_cross_axis_size( > > error ? kMinimumHeightForWhenTheresAnError : > > GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT)); > > > > I don't think it adds legibility. Do you? Or did you have something else in > > mind? > > If it is truly just a magic number that cannot be given a meaningful name, then > I agree my suggestion will be counterproductive. My original suggestion should > have read "... or give it a meaningful name". Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2498293003/#ps80001 (title: "is_active_user()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153,665987,658818,658825 ========== to ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153,665987,658818,658825 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153,665987,658818,658825 ========== to ========== CrOS md user row adjustments 1) make height of signed-in user row correct (extra padding above and below) 2) get rid of bubble-on-bubble for add user error case. Replace the row that says "Add new user" with error text instead. Also fix add user row size. 3) fix color of separator 4) fix width of sign out button 5) fix fonts of button, user name and user email 6) fix color and alpha of not-active-user rows (no grayscale avatar, whole row is 50% alpha) This does NOT fix non-active-user rows showing show the user name, (they don't currently but they should) however that will somehow have to deal with the media indicator, so I'm punting. BUG=632153,665987,658818,658825 Committed: https://crrev.com/87842b52fc6b0a6343afeffc8a07307849d1e98c Cr-Commit-Position: refs/heads/master@{#432730} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/87842b52fc6b0a6343afeffc8a07307849d1e98c Cr-Commit-Position: refs/heads/master@{#432730} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
