Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137563002:
OneClickSigninBubbleView: Only show title if no error present. [Mac+Views] (Closed)
|
Created:
6 years, 11 months ago by Oren Blasberg Modified:
6 years, 10 months ago CC:
chromium-reviews, tim+watch_chromium.org, tfarina, rsimha+watch_chromium.org, haitaol+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionOneClickSigninBubbleView: Only show title if no error present.
For the error case:
- Hide the "You're now signed in to Chrome" title.
- Change "OK, got it!" to just "OK"
XIB changes:
- Connected the close (OK) button to the closeButton_ outlet, which was for some reason missing before.
Screenshots (disregard text of errors; samples were used):
- Mac: http://i.imgur.com/hPDAY8i.png
- Views: http://i.imgur.com/ZM9ktga.png
BUG=333988
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248618
Patch Set 1 #Patch Set 2 : Views: Update "OK" button label to just say "OK" when error message is present. #Patch Set 3 : Mac side changes. #
Total comments: 16
Patch Set 4 : Resolve conflict in cc file. #Patch Set 5 : Respond to a couple of groby's changes. #Patch Set 6 : Use NSHeight instead of size.height. #Patch Set 7 : Update cc file to compile again after a recent change #
Messages
Total messages: 29 (0 generated)
groby, can you do the mac half?
views LGTM
Looking. Sorry, was OOO.
https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/one_click_signin_view_controller.mm (right): https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:24: const CGFloat kTopErrorMsgMargin = 12; Please don't abbreviate. Also, what is this constant for, and why 12? https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:161: oldFrame.origin.y += [titleTextField_ frame].size.height; Use rectangle helpers instead: oldFrame = NSOffsetRect(oldFrame, 0, NSHeight([titleTextField_ frame])); https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:185: [titleTextField_ setStringValue:@""]; Can you set field values before doing the layout, and then just layout based on the contents? Also, if you want to hide the titleTextField, [titleTextField_ setHidden:YES] is a better choice, I think. https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:191: [[self view] setFrameSize:size]; If you do this as part of the layout flow, the change will be accounted for in totalYOffset/delta, and the final resizeViewWithoutAutoResizingSubviews will automatically do the right thing. https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:193: // Shift the message text up to where the title text used to be. Why shift the messageTextField at all? Won't the resizing of the outer view already make sure it's at the top? https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:202: NSRect cbFrame = [closeButton_ frame]; Please don't use abbreviations. closeButtonFrame would be a good name. https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:206: [closeButton_ setFrame:fittedCbFrame]; Actually, since the view is already in a GTMWidthBasedTweaker, you could just use the layout tweaker. Make sure the button/tweaker is anchored to the bottom right. [GTMUILocalizerAndLayoutTweaker sizeToFitView:[closeButton_ superview]];
Thanks so much for the review! Ideally I would ask about some of these things in person but unfortunately I am out sick currently.. :) https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/one_click_signin_view_controller.mm (right): https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:161: oldFrame.origin.y += [titleTextField_ frame].size.height; On 2014/01/27 21:28:18, groby wrote: > Use rectangle helpers instead: > > oldFrame = NSOffsetRect(oldFrame, 0, NSHeight([titleTextField_ frame])); Done. https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:185: [titleTextField_ setStringValue:@""]; On 2014/01/27 21:28:18, groby wrote: > Can you set field values before doing the layout, and then just layout based on > the contents? > > Also, if you want to hide the titleTextField, [titleTextField_ setHidden:YES] is > a better choice, I think. Could you elaborate on what you mean by "doing the layout" and "layout based on the contents"? https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:191: [[self view] setFrameSize:size]; On 2014/01/27 21:28:18, groby wrote: > If you do this as part of the layout flow, the change will be accounted for in > totalYOffset/delta, and the final resizeViewWithoutAutoResizingSubviews will > automatically do the right thing. Same here: what do you mean by layout flow? Are you referring to the ShiftOriginY / totalYOffset stuff above? https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:193: // Shift the message text up to where the title text used to be. On 2014/01/27 21:28:18, groby wrote: > Why shift the messageTextField at all? Won't the resizing of the outer view > already make sure it's at the top? That is not happening for some reason. :/ Perhaps addressing your above comments regarding layout will help make this work? https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:202: NSRect cbFrame = [closeButton_ frame]; On 2014/01/27 21:28:18, groby wrote: > Please don't use abbreviations. closeButtonFrame would be a good name. Removed. https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:206: [closeButton_ setFrame:fittedCbFrame]; On 2014/01/27 21:28:18, groby wrote: > Actually, since the view is already in a GTMWidthBasedTweaker, you could just > use the layout tweaker. Make sure the button/tweaker is anchored to the bottom > right. > > [GTMUILocalizerAndLayoutTweaker sizeToFitView:[closeButton_ superview]]; Done.
Happy to explain the layout thing in person. It's a personal nit, though - the layout code works as is. Your pick. https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/one_click_signin_view_controller.mm (right): https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:185: [titleTextField_ setStringValue:@""]; On 2014/01/29 23:52:19, Oren Blasberg wrote: > On 2014/01/27 21:28:18, groby wrote: > > Can you set field values before doing the layout, and then just layout based > on > > the contents? > > > > Also, if you want to hide the titleTextField, [titleTextField_ setHidden:YES] > is > > a better choice, I think. > > Could you elaborate on what you mean by "doing the layout" and "layout based on > the contents"? "Doing the layout" ==> Arranging where fields show up. "Layout based on contents" ==> Happens after individual controls are sized properly. You now know the size of all elements based on what they contain, and can arrange them in one go. https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:191: [[self view] setFrameSize:size]; On 2014/01/29 23:52:19, Oren Blasberg wrote: > On 2014/01/27 21:28:18, groby wrote: > > If you do this as part of the layout flow, the change will be accounted for in > > totalYOffset/delta, and the final resizeViewWithoutAutoResizingSubviews will > > automatically do the right thing. > > Same here: what do you mean by layout flow? Are you referring to the > ShiftOriginY / totalYOffset stuff above? Yep - arranging individual controls so they look nice. There, you can just skip arranging the title field at all if it is hidden. Makes sense? (If it doesn't, let's sit down for a sec. ) https://codereview.chromium.org/137563002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:193: // Shift the message text up to where the title text used to be. On 2014/01/29 23:52:19, Oren Blasberg wrote: > On 2014/01/27 21:28:18, groby wrote: > > Why shift the messageTextField at all? Won't the resizing of the outer view > > already make sure it's at the top? > > That is not happening for some reason. :/ Perhaps addressing your above comments > regarding layout will help make this work? Weird. If the field is outside the view's bounds, it shouldn't render.
As discussed offline, the weird layout calculations were necessary because the bubble is made slightly less tall in the error case, to make up for the title text being missing.
With that, LGTM.
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/orenb@chromium.org/137563002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/orenb@chromium.org/137563002/430001
The CQ bit was unchecked by commit-bot@chromium.org
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 bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/orenb@chromium.org/137563002/430001
Message was sent while issue was closed.
Change committed as 248618
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
