Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
132903016:
[Mac] UI fixes to the new avatar bubble. (Closed)
|
Created:
6 years, 10 months ago by noms (inactive) Modified:
6 years, 10 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] UI fixes to the new avatar bubble.
- the bottom options buttons get a blue background when hovered over
- corrected the spacing between a button's image and its title.
- s/makeAccountButtonWithRect/accountButtonWithRect/g to be
consistent with the other control-making methods.
Screenshot: https://drive.google.com/file/d/0B1B1Up4p2NRMeERscUNleHNMWWc/edit?usp=sharing
BUG=324036
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247910
Patch Set 1 : #
Total comments: 24
Patch Set 2 : rachel comments #Patch Set 3 : rebase from hell attempt #
Total comments: 2
Patch Set 4 : s/BOOL/bool/ #Messages
Total messages: 11 (0 generated)
Hi Rachel, owner of Cocoas :) I've cleaned up the UI on the new avatar bubble to match the mocks (see screenshot). Please take a look. Also, to pre-empt a potential question: I have an open bug to move the custom controls out of this class (as it's getting monstruos) and into their own files, but it first requires to move all the profile/avatar related code in a cocoa subdirectory, so I wanted to land all of my in-progress CLs first.
https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (left): https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:647: [button sizeToFit]; Since you don't sizeToFit any more - what about long text? Do we elide? https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:154: int imageWidth_; Not used. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:188: NSSize textSize = [super cellSize]; I'd suggest 'buttonSize' instead - it's more than just the text. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:216: BOOL isHighlighted = [self hoverState] != kHoverStateNone; Personal nit - parentheses around expression. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:221: [backgroundColor setFill]; Your button is borderless - in which case it's much easier to just override setHoverState to change the cell's background color dependend on hover state. Less custom drawing is good :) https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:227: @interface ProfileChooserController (Private) FWIW - private details are usually in a category with an empty name. I.e. @interface ProfileChooserController() https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:257: - (BackgroundColorHoverButton*)hoverButtonWithRect:(NSRect)rect No need to change the type here. AFAICT, all the consumers are cool with an NSButton, so keeping the actual type of button an implementation detail would be good. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:257: - (BackgroundColorHoverButton*)hoverButtonWithRect:(NSRect)rect Naming in the class is inconsistent - you create some things with createXXX, and these functions here look like accessors instead. (This kind of name would be appropriate if this were a class method/factory though) https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:423: NSView* optionsView = [self createOptionsViewWithRect:NSMakeRect( nit: pull NSMakeRect( onto the next line. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:449: [self createCurrentProfileAccountsView:NSMakeRect( see above https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:631: [container setFrameSize:NSMakeSize(NSWidth([container frame]), Create container here. rect.size.height = NSMaxY(... ... container([[NSView alloc] initiWithFrame:rect]); skips unnecessary resize. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:731: [button setAlignment:NSLeftTextAlignment]; default is NSNaturalTextAlignment - which already is left for LTR languages and switches to right for RTL languages. I suspect that is a better choice than "always left"
Addressed (all - 1) of the comments + asked a question for one of them. There was also a really unfortunate rebase in the middle, which I hope didn't break anything. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (left): https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:647: [button sizeToFit]; I have a separate CL where the bubble has to be converted to always be a fixed size, so a ton of eliding is done: https://codereview.chromium.org/143743005/ On 2014/01/28 20:05:22, groby wrote: > Since you don't sizeToFit any more - what about long text? Do we elide? https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:154: int imageWidth_; Oops. Done. On 2014/01/28 20:05:22, groby wrote: > Not used. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:188: NSSize textSize = [super cellSize]; On 2014/01/28 20:05:22, groby wrote: > I'd suggest 'buttonSize' instead - it's more than just the text. Done. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:216: BOOL isHighlighted = [self hoverState] != kHoverStateNone; On 2014/01/28 20:05:22, groby wrote: > Personal nit - parentheses around expression. Done. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:221: [backgroundColor setFill]; AMAZING. I didn't know that. Done! On 2014/01/28 20:05:22, groby wrote: > Your button is borderless - in which case it's much easier to just override > setHoverState to change the cell's background color dependend on hover state. > Less custom drawing is good :) https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:227: @interface ProfileChooserController (Private) On 2014/01/28 20:05:22, groby wrote: > FWIW - private details are usually in a category with an empty name. I.e. > > @interface ProfileChooserController() Done. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:257: - (BackgroundColorHoverButton*)hoverButtonWithRect:(NSRect)rect The functions used to be called "makeLinkButton" etc, but Nico had commented in a past CL (https://codereview.chromium.org/102913002/) that I should take away the 'make' as they return autoreleased objects. For the giant functions I left the "create" verb in -- should i take it away from those too? On 2014/01/28 20:05:22, groby wrote: > Naming in the class is inconsistent - you create some things with createXXX, and > these functions here look like accessors instead. (This kind of name would be > appropriate if this were a class method/factory though) https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:257: - (BackgroundColorHoverButton*)hoverButtonWithRect:(NSRect)rect On 2014/01/28 20:05:22, groby wrote: > No need to change the type here. AFAICT, all the consumers are cool with an > NSButton, so keeping the actual type of button an implementation detail would be > good. Done. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:423: NSView* optionsView = [self createOptionsViewWithRect:NSMakeRect( On 2014/01/28 20:05:22, groby wrote: > nit: pull NSMakeRect( onto the next line. Done in a bunch of places. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:449: [self createCurrentProfileAccountsView:NSMakeRect( On 2014/01/28 20:05:22, groby wrote: > see above Done. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:631: [container setFrameSize:NSMakeSize(NSWidth([container frame]), On 2014/01/28 20:05:22, groby wrote: > Create container here. > rect.size.height = NSMaxY(... > ... container([[NSView alloc] initiWithFrame:rect]); > > skips unnecessary resize. Done. https://codereview.chromium.org/132903016/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:731: [button setAlignment:NSLeftTextAlignment]; If I don't set it explicitly, the text is displayed sort of off-center to the right -- it's neither centered, nor justified, nor right aligned. But it's definitely not left aligned. I'm init'ing with a rect, so I don't know what bounds I am missing :( (On 2014/01/28 20:05:22, groby wrote: > default is NSNaturalTextAlignment - which already is left for LTR languages and > switches to right for RTL languages. I suspect that is a better choice than > "always left"
lgtm https://codereview.chromium.org/132903016/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/132903016/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:413: BOOL isHighlighted = ([self hoverState] != kHoverStateNone); You technically want bool, not BOOL :) (Since it's used in a ternary, not by a Cocoa method)
Thanks! https://codereview.chromium.org/132903016/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/132903016/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:413: BOOL isHighlighted = ([self hoverState] != kHoverStateNone); On 2014/01/29 20:06:54, groby wrote: > You technically want bool, not BOOL :) (Since it's used in a ternary, not by a > Cocoa method) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/132903016/120001
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/132903016/120001
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/132903016/120001
Message was sent while issue was closed.
Change committed as 247910 |
