Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
585473004:
Checkbox alignment & bigger height for Uma opt-in view. (Closed)
DescriptionCheckbox alignment & bigger height for Uma opt-in view.
1. Move the text next to the checkbox down by 1px to make it look aligned.
2. Add 2 more px to the butom of the uma opt-in view.
BUG=293702
Committed: https://crrev.com/a160746d4a7dfac2a70893fdc7fef2bfd041c1d1
Cr-Commit-Position: refs/heads/master@{#298988}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Address comments. #
Total comments: 7
Patch Set 3 : Address comments #Messages
Total messages: 19 (7 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
yiyaoliu@chromium.org changed reviewers: + msw@chromium.org
Please post before/after pictures with UI fixes like this. https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:59: const int kUMAOptinViewBottomInset = 10; Move this down to where it's used and change the comment to explain why it's necessary, the identifier already describes what it is. https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:208: offer_uma_optin_(true), Why are you changing this? Was this intentional? https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:344: uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0)); Why is this and the uma_layout inset change needed? Shouldn't one or the other be sufficient?
Patchset #2 (id:60001) has been deleted
Requirement from @ainslie: [image: Inline image 1] Before (windows): [image: Inline image 2] After (windows): [image: Inline image 2] Thanks, Yiyao On Thu, Sep 18, 2014 at 7:14 PM, <msw@chromium.org> wrote: > Please post before/after pictures with UI fixes like this. > > > https://codereview.chromium.org/585473004/diff/40001/ > chrome/browser/ui/views/session_crashed_bubble_view.cc > File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): > > https://codereview.chromium.org/585473004/diff/40001/ > chrome/browser/ui/views/session_crashed_bubble_view.cc#newcode59 > chrome/browser/ui/views/session_crashed_bubble_view.cc:59: const int > kUMAOptinViewBottomInset = 10; > Move this down to where it's used and change the comment to explain why > it's necessary, the identifier already describes what it is. > > https://codereview.chromium.org/585473004/diff/40001/ > chrome/browser/ui/views/session_crashed_bubble_view.cc#newcode208 > chrome/browser/ui/views/session_crashed_bubble_view.cc:208: > offer_uma_optin_(true), > Why are you changing this? Was this intentional? > > https://codereview.chromium.org/585473004/diff/40001/ > chrome/browser/ui/views/session_crashed_bubble_view.cc#newcode344 > chrome/browser/ui/views/session_crashed_bubble_view.cc:344: > uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0)); > Why is this and the uma_layout inset change needed? Shouldn't one or the > other be sufficient? > > https://codereview.chromium.org/585473004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:59: const int kUMAOptinViewBottomInset = 10; On 2014/09/18 23:14:48, msw wrote: > Move this down to where it's used and change the comment to explain why it's > necessary, the identifier already describes what it is. There is no particular reason, originally value 8 (from views::kRelatedControlVerticalSpacing) was used, but the UI designer wants it to be 10px now. https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:208: offer_uma_optin_(true), On 2014/09/18 23:14:48, msw wrote: > Why are you changing this? Was this intentional? Done. https://codereview.chromium.org/585473004/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:344: uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0)); On 2014/09/18 23:14:48, msw wrote: > Why is this and the uma_layout inset change needed? Shouldn't one or the other > be sufficient? They are separate things. Changed the issue title to make this clearer. This is for the checkbox alignment. The inset change is to make the empty region at bottom of the UMA opt-in view higher (2 more pixels).
lgtm with nits. https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:58: // Bottom inset for UMA opt-in view in pixels. nit: be consistent about "optin" vs. "opt-in" in comments. https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:59: const int kUMAOptinViewBottomInset = 10; nit: This could be moved into the scope of the one function where it's used, immediately preceding its use. https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:342: // Since the checkbox has a bigger height than text, moves it down by 1px to nit: consider "Shift the text down by 1px to align with the checkbox."
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:344: uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0)); This only works with the default font on windows. If we're we want the checkbox image to go from the font baseline to the font ascent, then we'll need to draw the image in code. I don't think you should worry about futzing with the checkbox image here. It should be done in the checkbox code.
On 2014/09/22 15:06:34, sky wrote: > https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... > File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): > > https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... > chrome/browser/ui/views/session_crashed_bubble_view.cc:344: > uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0)); > This only works with the default font on windows. > If we're we want the checkbox image to go from the font baseline to the font > ascent, then we'll need to draw the image in code. > I don't think you should worry about futzing with the checkbox image here. It > should be done in the checkbox code. But since the checkbox only supports plain text (and we need a hyperlink in the text), the checkbox image and the text to the right of it are separate views. Therefore I have to handle the alignment by myself I think. It's a bit of hacky but I couldn't think of a better way to make part of the text of a checkbox as a link. On a second thought, the whole checkbox text should be clickable with the same effect as clicking the checkbox image... plugging a hyperlink into that text wouldn't make a lot of sense...
On 2014/09/22 15:23:32, yao wrote: > On 2014/09/22 15:06:34, sky wrote: > > > https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... > > File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): > > > > > https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... > > chrome/browser/ui/views/session_crashed_bubble_view.cc:344: > > uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0)); > > This only works with the default font on windows. > > If we're we want the checkbox image to go from the font baseline to the font > > ascent, then we'll need to draw the image in code. > > I don't think you should worry about futzing with the checkbox image here. It > > should be done in the checkbox code. > > But since the checkbox only supports plain text (and we need a hyperlink in the > text), the checkbox image and the text to the right of it are separate views. > Therefore I have to handle the alignment by myself I think. It's a bit of hacky > but I couldn't think of a better way to make part of the text of a checkbox as a > link. You should be able to get that effect by setting the font of the checkbox to match the richer of the fonts you want. I layout is based on width (I think it is), then you may need to subclass to precisely align. None-the-less precise alignment isn't available while we use an image. So, again, I would worry about trying to get a precise alignment for this patch. Right fix for that is in checkbox. > On a second thought, the whole checkbox text should be clickable with the same > effect as clicking the checkbox image... plugging a hyperlink into that text > wouldn't make a lot of sense... I agree on the clicking.
Patchset #3 (id:100001) has been deleted
Will submit after test. https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/session_crashed_bubble_view.cc (right): https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:58: // Bottom inset for UMA opt-in view in pixels. On 2014/09/19 18:01:48, msw wrote: > nit: be consistent about "optin" vs. "opt-in" in comments. Done. https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:59: const int kUMAOptinViewBottomInset = 10; On 2014/09/19 18:01:48, msw wrote: > nit: This could be moved into the scope of the one function where it's used, > immediately preceding its use. Done. https://codereview.chromium.org/585473004/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/session_crashed_bubble_view.cc:342: // Since the checkbox has a bigger height than text, moves it down by 1px to On 2014/09/19 18:01:48, msw wrote: > nit: consider "Shift the text down by 1px to align with the checkbox." Done.
The CQ bit was checked by yiyaoliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/585473004/160001
Message was sent while issue was closed.
Committed patchset #3 (id:160001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a160746d4a7dfac2a70893fdc7fef2bfd041c1d1 Cr-Commit-Position: refs/heads/master@{#298988} |
