Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197253002:
[Mac][MD User Menu] UI Tweaks (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] UI Tweaks
Specifically:
1. Fixed misalignment between user menu and avatar button (see bug #1)
2. Fixed pixelated profile icon border (see bug #2)
3. Fixed certain letters being cut off on the bottom for avatar name
4. Disabled the color change for button icons upon mouse down
5. Disabled focus for buttons that are disabled (see bug #3)
See comparative screenshots:
https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8
BUG=632936
BUG=633175
BUG=633877
BUG=615893
Committed: https://crrev.com/bd2995fb57c078db3de42e5ad46faf5ad428e80d
Cr-Commit-Position: refs/heads/master@{#411323}
Patch Set 1 #Patch Set 2 : Number tweaking #
Total comments: 11
Patch Set 3 : Removed press effect on button icons #Patch Set 4 : Put the change behind flag #
Total comments: 5
Patch Set 5 : NSHeight + rebased #
Messages
Total messages: 25 (16 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button 2. Fixed pixelated profile icon border (see bug) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Added press effect for the profile icon in the profile card button to stay consistent with the other option buttons on mac. See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=633175 ========== to ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button on 2x display 2. Fixed pixelated profile icon border (see bug) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Added press effect for the profile icon in the profile card button to stay consistent with the other option buttons on mac. See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=633175 ==========
janeliulwq@google.com changed reviewers: + groby@chromium.org
janeliulwq@google.com changed required reviewers: + groby@chromium.org
Hi Rachel, PTAL, thanks!
Description was changed from ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button on 2x display 2. Fixed pixelated profile icon border (see bug) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Added press effect for the profile icon in the profile card button to stay consistent with the other option buttons on mac. See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=633175 ========== to ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Added press effect for the profile icon in the profile card button to stay consistent with the other option buttons on mac. See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=615893 ==========
https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:575: profiles::SHAPE_SQUARE)]; I'm confused - the "after" images still show a round avatar? (Also, did we get rid of the MD menu?) https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1919: [[profileCard cell] setImageDimsWhenDisabled:NO]; How will the user tell it's disabled? https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1929: // smaller versions of them (24 x 24) to adapt to smaller profile icons. Note (not this CL): You probably want SVG images here. https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1956: // Adding 1px in height to avoid cutting off certain letters on the bottom. That's weird - any chance the letters are cut off because the offset was at a half-pixel? (You might want to use std::ceil/floor for any arithmetic on coords that's potentially non-Integer)
Description was changed from ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Added press effect for the profile icon in the profile card button to stay consistent with the other option buttons on mac. See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=615893 ========== to ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Added press effect for the profile icon in the profile card button to stay consistent with the other option buttons on mac 5. Disabled focus for buttons that are disabled (see bug #3) See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=633877 BUG=615893 ==========
Description was changed from ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Added press effect for the profile icon in the profile card button to stay consistent with the other option buttons on mac 5. Disabled focus for buttons that are disabled (see bug #3) See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=633877 BUG=615893 ========== to ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Disabled the color change for button icons upon mouse down 5. Disabled focus for buttons that are disabled (see bug #3) See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=633877 BUG=615893 ==========
Hi Rachel, thanks for the review! I hope it's ok that I just added two more very small tweaks in this CL (see #4 & #5 in the desc, I updated the screenshots too). PTAL, thanks! https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:575: profiles::SHAPE_SQUARE)]; On 2016/08/03 17:23:09, groby wrote: > I'm confused - the "after" images still show a round avatar? (Also, did we get > rid of the MD menu?) Yes it's still a round avatar, and no we are keeping the MD menu (I mean, I'm currently implementing it). Explanation: This class "EditableProfilePhoto" is actually for non-MD user menu only (the old menu); the "kMdImageSide" that wrongly appeared here probably came from my draft code before and didn't get cleaned up... This class displays a round avatar by using a circular CustomCircleImageCell. However, since profiles::GetSizedAvatarIcon can now directly return a round avatar, in the last CL, I changed the function call to be SHAPE_CIRCLE for the sole purpose of having more straightforward code. However, it turns out that SHAPE_CIRCLE causes the camera icon overlay to have pixelated border... That's why I'm switching back to SHAPE_SQUARE for now. The avatar would still be round thanks to the cell. It probably sounds weird and confusing, but at least we will be removing this class later once MD menu is ready. https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1919: [[profileCard cell] setImageDimsWhenDisabled:NO]; On 2016/08/03 17:23:09, groby wrote: > How will the user tell it's disabled? Double checked with UI folks about this design: "The button has an informative (showing the avatar) function as well as being a link to settings, so it's not "disabled" in the same sense even when it's not linking to anything." Hope this makes sense! https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1929: // smaller versions of them (24 x 24) to adapt to smaller profile icons. On 2016/08/03 17:23:09, groby wrote: > Note (not this CL): You probably want SVG images here. Yes, I will be using SVG images! :) I'm just still waiting for new icon resources to be made. https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1956: // Adding 1px in height to avoid cutting off certain letters on the bottom. On 2016/08/03 17:23:09, groby wrote: > That's weird - any chance the letters are cut off because the offset was at a > half-pixel? (You might want to use std::ceil/floor for any arithmetic on coords > that's potentially non-Integer) Woah, yes that's exactly why! Changed it to std::floor instead of having a weird "1". Thanks!
LGTM https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1919: [[profileCard cell] setImageDimsWhenDisabled:NO]; On 2016/08/08 19:22:24, Jane wrote: > On 2016/08/03 17:23:09, groby wrote: > > How will the user tell it's disabled? > > Double checked with UI folks about this design: > "The button has an informative (showing the avatar) function as well as being a > link to settings, so it's not "disabled" in the same sense even when it's not > linking to anything." > Hope this makes sense! Mumblegrumble UI should always indicate if there is an action behind it or not. I get that we don't want to mess with the Avatar picture, but could you ping UI for feedback on that (outside of this CL)? https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1956: // Adding 1px in height to avoid cutting off certain letters on the bottom. On 2016/08/08 19:22:24, Jane wrote: > On 2016/08/03 17:23:09, groby wrote: > > That's weird - any chance the letters are cut off because the offset was at a > > half-pixel? (You might want to use std::ceil/floor for any arithmetic on > coords > > that's potentially non-Integer) > > Woah, yes that's exactly why! Changed it to std::floor instead of having a weird > "1". Thanks! My pleasure. You have now passed the std::ceil rite-of-passage ;) (It literally happens to pretty much everybody laying out Cocoa things the first time they get non-integer coords. Your comment helped me realize that's what was happening!) https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1957: [profileName sizeToFit]; FWIW: You seem to use sizeToFit only to derive font height - it might be easier to just retrieve that and build the frame directly? I.e. via PlatformFontMac([profileName font]).GetHeight(); If I miss something, and there's another reason, it'd probably be good to add a comment. https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1963: [profileName frame].size.height)]; Tiny nit: NSHeight([profileName frame]) here and prev. line. (We're in general trying to not refer to members inside NSRect, but use the NSFoundation helpers, if at all possible);
Hi Rachel, one question below (comment #2), thanks! https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2197253002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1919: [[profileCard cell] setImageDimsWhenDisabled:NO]; On 2016/08/10 20:01:51, groby wrote: > On 2016/08/08 19:22:24, Jane wrote: > > On 2016/08/03 17:23:09, groby wrote: > > > How will the user tell it's disabled? > > > > Double checked with UI folks about this design: > > "The button has an informative (showing the avatar) function as well as being > a > > link to settings, so it's not "disabled" in the same sense even when it's not > > linking to anything." > > Hope this makes sense! > > Mumblegrumble UI should always indicate if there is an action behind it or not. > I get that we don't want to mess with the Avatar picture, but could you ping UI > for feedback on that (outside of this CL)? I will bring this up for UI review (coming soon)! Thanks. https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1957: [profileName sizeToFit]; On 2016/08/10 20:01:51, groby wrote: > FWIW: You seem to use sizeToFit only to derive font height - it might be easier > to just retrieve that and build the frame directly? I.e. via > PlatformFontMac([profileName font]).GetHeight(); > > If I miss something, and there's another reason, it'd probably be good to add a > comment. Yes, I'm only using sizeToFit to adapt to the new font size. However, using PlatformFontMac seems to be throwing linking error for me... I also tried just using gfx::Font([profileName font]).GetHeight(), but the height given seems to be a tiny little too small. Anything else I should try, or is it ok to keep the sizeToFit? Thanks! https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1963: [profileName frame].size.height)]; On 2016/08/10 20:01:51, groby wrote: > Tiny nit: NSHeight([profileName frame]) here and prev. line. (We're in general > trying to not refer to members inside NSRect, but use the NSFoundation helpers, > if at all possible); Done.
Still LGTM https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/2197253002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1957: [profileName sizeToFit]; On 2016/08/10 22:42:58, Jane wrote: > On 2016/08/10 20:01:51, groby wrote: > > FWIW: You seem to use sizeToFit only to derive font height - it might be > easier > > to just retrieve that and build the frame directly? I.e. via > > PlatformFontMac([profileName font]).GetHeight(); > > > > If I miss something, and there's another reason, it'd probably be good to add > a > > comment. > > Yes, I'm only using sizeToFit to adapt to the new font size. > However, using PlatformFontMac seems to be throwing linking error for me... I > also tried just using gfx::Font([profileName font]).GetHeight(), but the height > given seems to be a tiny little too small. > Anything else I should try, or is it ok to keep the sizeToFit? > Thanks! It _should_ work fine, but since it doesn't, just skip it. (I assume that NSTextfield adds some space somewhere. It's not worth digging into, since sizeToFit does the same thing)
Patchset #5 (id:100001) has been deleted
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/2197253002/#ps120001 (title: "NSHeight + rebased")
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 ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Disabled the color change for button icons upon mouse down 5. Disabled focus for buttons that are disabled (see bug #3) See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=633877 BUG=615893 ========== to ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Disabled the color change for button icons upon mouse down 5. Disabled focus for buttons that are disabled (see bug #3) See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=633877 BUG=615893 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Disabled the color change for button icons upon mouse down 5. Disabled focus for buttons that are disabled (see bug #3) See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=633877 BUG=615893 ========== to ========== [Mac][MD User Menu] UI Tweaks Specifically: 1. Fixed misalignment between user menu and avatar button (see bug #1) 2. Fixed pixelated profile icon border (see bug #2) 3. Fixed certain letters being cut off on the bottom for avatar name 4. Disabled the color change for button icons upon mouse down 5. Disabled focus for buttons that are disabled (see bug #3) See comparative screenshots: https://drive.google.com/corp/drive/folders/0B7Fvv7JszRyGWUtYM29OdnVjQm8 BUG=632936 BUG=633175 BUG=633877 BUG=615893 Committed: https://crrev.com/bd2995fb57c078db3de42e5ad46faf5ad428e80d Cr-Commit-Position: refs/heads/master@{#411323} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bd2995fb57c078db3de42e5ad46faf5ad428e80d Cr-Commit-Position: refs/heads/master@{#411323} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
