Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
469653002:
Align the checkbox with the first line of the text. (Closed)
DescriptionAlign the checkbox with the first line of the text.
BUG=293702
Committed: https://crrev.com/1ee769efa69e99938c922d8925328190bb1c9097
Cr-Commit-Position: refs/heads/master@{#292391}
Patch Set 1 : #Patch Set 2 : Make the backgroud color consistent in the button bar. #Patch Set 3 : Nits. #Patch Set 4 : Nits #
Total comments: 16
Patch Set 5 : Address comments. #
Total comments: 3
Messages
Total messages: 20 (0 generated)
LGTM, but I'll defer to Mike's views expertise here.
LGTM; seems correct, but I'd really like before/after pics.
before: [image: Inline image 2] after: [image: Inline image 1] On Wed, Aug 13, 2014 at 3:44 PM, <msw@chromium.org> wrote: > LGTM; seems correct, but I'd really like before/after pics. > > https://codereview.chromium.org/469653002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It looks like the after image has a background color regression. There's a vertical line between the checkbox and text where the background color changes. On Wed, Aug 13, 2014 at 1:04 PM, Yiyao Liu <yiyaoliu@google.com> wrote: > before: > [image: Inline image 2] > > after: > [image: Inline image 1] > > > > On Wed, Aug 13, 2014 at 3:44 PM, <msw@chromium.org> wrote: > >> LGTM; seems correct, but I'd really like before/after pics. >> >> https://codereview.chromium.org/469653002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
You are right, parts circled in back square didn't get set background color. Is there a way to set a background color for the whole column set? [image: Inline image 1] Also, I don't understand why there's a small column there to the right most. No matter I increase/decrease the right border value, that greyish column always exist. [image: Inline image 3] On Wed, Aug 13, 2014 at 7:12 PM, Michael Wasserman <msw@chromium.org> wrote: > It looks like the after image has a background color regression. > There's a vertical line between the checkbox and text where the background > color changes. > > > On Wed, Aug 13, 2014 at 1:04 PM, Yiyao Liu <yiyaoliu@google.com> wrote: > >> before: >> [image: Inline image 2] >> >> after: >> [image: Inline image 1] >> >> >> >> On Wed, Aug 13, 2014 at 3:44 PM, <msw@chromium.org> wrote: >> >>> LGTM; seems correct, but I'd really like before/after pics. >>> >>> https://codereview.chromium.org/469653002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
You can't set background colors on GridLayout itself, it's just away to layout views. The horizontal separator line extends to the bubble border, so something about your other GridLayout column set (or the Views it contains) is probably wrong and preventing that View's visible content from extending to the edge. (you probably don't need to use GridLayout::SetInsets; and Init's set_margins(gfx::Insets()) looks correct, but that may also contribute to the corners not looking round). On Thu, Aug 14, 2014 at 8:05 AM, Yiyao Liu <yiyaoliu@google.com> wrote: > You are right, parts circled in back square didn't get set background > color. > Is there a way to set a background color for the whole column set? > > [image: Inline image 1] > > Also, I don't understand why there's a small column there to the right > most. > No matter I increase/decrease the right border value, that greyish column > always exist. > [image: Inline image 3] > > > > > On Wed, Aug 13, 2014 at 7:12 PM, Michael Wasserman <msw@chromium.org> > wrote: > >> It looks like the after image has a background color regression. >> There's a vertical line between the checkbox and text where the >> background color changes. >> >> >> On Wed, Aug 13, 2014 at 1:04 PM, Yiyao Liu <yiyaoliu@google.com> wrote: >> >>> before: >>> [image: Inline image 2] >>> >>> after: >>> [image: Inline image 1] >>> >>> >>> >>> On Wed, Aug 13, 2014 at 3:44 PM, <msw@chromium.org> wrote: >>> >>>> LGTM; seems correct, but I'd really like before/after pics. >>>> >>>> https://codereview.chromium.org/469653002/ >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #2 (id:40001) has been deleted
The backgroud color problem should be fixed now.
screen shot: [image: Inline image 2] [image: Inline image 1] On Tue, Aug 26, 2014 at 11:19 AM, <yiyaoliu@chromium.org> wrote: > The backgroud color problem should be fixed now. > > https://codereview.chromium.org/469653002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looks pretty good, just some minor nits. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:285: int buttom_margin = 1; nit: "bottom". https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:289: const int kUmaOptionColumnSetId = 2; nit: capitalize UMA here. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:293: // Add a separator. nit: remove superfluous comment. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:296: // Add the view that lets user opt-in to UMA. nit: remove superfluous comment. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:300: // Since the uma optin row has a different background than the default nit: capitalize UMA here. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:344: views::View* sub_view = new views::View(); optional nit: consider |uma_view| or |optin_view|; ditto for |sub_layout|. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:101: // Create the view for user to opt in to Uma. nit: "for the user", s/Uma/UMA. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:102: views::View* CreateUmaOptinView(); nit: capitalize UMA in the function name, that seems more common.
https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:285: int buttom_margin = 1; On 2014/08/26 20:19:37, msw wrote: > nit: "bottom". Done. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:289: const int kUmaOptionColumnSetId = 2; On 2014/08/26 20:19:37, msw wrote: > nit: capitalize UMA here. Done. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:293: // Add a separator. On 2014/08/26 20:19:37, msw wrote: > nit: remove superfluous comment. Done. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:296: // Add the view that lets user opt-in to UMA. On 2014/08/26 20:19:37, msw wrote: > nit: remove superfluous comment. Done. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:300: // Since the uma optin row has a different background than the default On 2014/08/26 20:19:37, msw wrote: > nit: capitalize UMA here. Done. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:344: views::View* sub_view = new views::View(); On 2014/08/26 20:19:37, msw wrote: > optional nit: consider |uma_view| or |optin_view|; ditto for |sub_layout|. Done. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.h (right): https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:101: // Create the view for user to opt in to Uma. On 2014/08/26 20:19:37, msw wrote: > nit: "for the user", s/Uma/UMA. Done. https://codereview.chromium.org/469653002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.h:102: views::View* CreateUmaOptinView(); On 2014/08/26 20:19:37, msw wrote: > nit: capitalize UMA in the function name, that seems more common. Done.
lgtm with a nit. https://codereview.chromium.org/469653002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/469653002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:289: const int kUMAOptionColumnSetId = 2; nit: should this be "Optin" instead of "Option"?
https://codereview.chromium.org/469653002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/469653002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:289: const int kUMAOptionColumnSetId = 2; On 2014/08/27 23:43:07, msw wrote: > nit: should this be "Optin" instead of "Option"? I wanted it to be Option, (The checkout box is named uma_option, I was thinking it's an option to optin to UMA). If you think it makes more sense to be optin, I'll change it.
https://codereview.chromium.org/469653002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/469653002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/session_crashed_bubble_view.cc:289: const int kUMAOptionColumnSetId = 2; On 2014/08/28 00:13:36, yao wrote: > On 2014/08/27 23:43:07, msw wrote: > > nit: should this be "Optin" instead of "Option"? > > I wanted it to be Option, (The checkout box is named uma_option, I was thinking > it's an option to optin to UMA). If you think it makes more sense to be optin, > I'll change it. It's fine as-is if that what you intended.
The CQ bit was checked by yiyaoliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/469653002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as e46790f00b28fd21193be1b4d4c611d8f06a46f6
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1ee769efa69e99938c922d8925328190bb1c9097 Cr-Commit-Position: refs/heads/master@{#292391} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
