Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2267723002:
[Mac][MD User Menu] Updated the profile badge icons (Closed)
|
Created:
4 years, 4 months ago by Jane Modified:
4 years, 4 months ago Reviewers:
*groby-ooo-7-16 CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac][MD User Menu] Updated the profile badge icons
Instead of using the legacy png resources, MD user menu now uses new svg
icons for profile badges for child and supervised users. A circular
background of the same color of the user menu background is drawn first,
and then the badge icon on top of it.
Screenshot: https://drive.google.com/open?id=0B7Fvv7JszRyGLWR2WG5HazRma1k
Mock (different icons though): https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20Sign%20In/user_menu/preview#%2Fpreview-2.png
BUG=615893
Committed: https://crrev.com/441da708943b005941af140a1364c0b305eafa4c
Cr-Commit-Position: refs/heads/master@{#413760}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Nits #
Total comments: 2
Patch Set 3 : One more #Messages
Total messages: 18 (10 generated)
janeliulwq@google.com changed reviewers: + groby@chromium.org
janeliulwq@google.com changed required reviewers: + groby@chromium.org
Thanks!
Nice! A few nits, and one question re: formatting, but LGTM once that's addressed. https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2092: const int badgeSize = 24; Tiny nit: Maybe want to use constexpr here. (It's a tiny semantic difference - constexpr is guaranteed to be known at compiletime - but it seems to be what Chrome is doing for constants these days) https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2096: initWithFrame:NSMakeRect( is that formatted with git cl format? I'd think the method call would need to be indented 4 here, but cl format is occasionally odd with long lines of code. https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2097: xOffset + kMdImageSide - badgeSize + badgeSpacing, Maybe factor out either the entire rect, or at least the shared offset? I know, it's ObjectiveC, lines _should_ be long, but maybe a bit shorter is good :) https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2101: // Draw the badge icon. Super nitpicky nit: Add the badge icon - you're not drawing here.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2092: const int badgeSize = 24; On 2016/08/23 02:32:55, groby wrote: > Tiny nit: Maybe want to use constexpr here. (It's a tiny semantic difference - > constexpr is guaranteed to be known at compiletime - but it seems to be what > Chrome is doing for constants these days) Done. https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2096: initWithFrame:NSMakeRect( On 2016/08/23 02:32:55, groby wrote: > is that formatted with git cl format? I'd think the method call would need to be > indented 4 here, but cl format is occasionally odd with long lines of code. True, I see... https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2097: xOffset + kMdImageSide - badgeSize + badgeSpacing, On 2016/08/23 02:32:55, groby wrote: > Maybe factor out either the entire rect, or at least the shared offset? I know, > it's ObjectiveC, lines _should_ be long, but maybe a bit shorter is good :) Done. Factored out the rect above. https://codereview.chromium.org/2267723002/diff/1/chrome/browser/ui/cocoa/pro... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2101: // Draw the badge icon. On 2016/08/23 02:32:55, groby wrote: > Super nitpicky nit: Add the badge icon - you're not drawing here. Done.
Still LGTM https://codereview.chromium.org/2267723002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2267723002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2102: const int borderWidth = 1; Missed one constexpr :)
https://codereview.chromium.org/2267723002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2267723002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2102: const int borderWidth = 1; On 2016/08/23 15:07:55, groby wrote: > Missed one constexpr :) Oops, thanks!
The CQ bit was checked by janeliulwq@google.com 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.
The CQ bit was checked by janeliulwq@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/2267723002/#ps60001 (title: "One more")
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.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Mac][MD User Menu] Updated the profile badge icons Instead of using the legacy png resources, MD user menu now uses new svg icons for profile badges for child and supervised users. A circular background of the same color of the user menu background is drawn first, and then the badge icon on top of it. Screenshot: https://drive.google.com/open?id=0B7Fvv7JszRyGLWR2WG5HazRma1k Mock (different icons though): https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 ========== to ========== [Mac][MD User Menu] Updated the profile badge icons Instead of using the legacy png resources, MD user menu now uses new svg icons for profile badges for child and supervised users. A circular background of the same color of the user menu background is drawn first, and then the badge icon on top of it. Screenshot: https://drive.google.com/open?id=0B7Fvv7JszRyGLWR2WG5HazRma1k Mock (different icons though): https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S... BUG=615893 Committed: https://crrev.com/441da708943b005941af140a1364c0b305eafa4c Cr-Commit-Position: refs/heads/master@{#413760} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/441da708943b005941af140a1364c0b305eafa4c Cr-Commit-Position: refs/heads/master@{#413760} |
