Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2148963002:
BubbleFrameView: add top padding even when close button is hidden (Closed)
|
Created:
4 years, 5 months ago by Elly Fong-Jones Modified:
4 years, 5 months ago Reviewers:
msw CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBubbleFrameView: add top padding even when close button is hidden
If there's no title or close button present, BubbleFrameView still needs some
top padding to avoid the content being hard up against the top edge. Always pad
as though the close button was visible, even if it's not, unless there's a
title.
BUG=622859
Committed: https://crrev.com/f06ef4ac6ac3dd2f3a76486d2efde9ec35583ae6
Cr-Commit-Position: refs/heads/master@{#406856}
Patch Set 1 #
Total comments: 2
Patch Set 2 : use min_height #Patch Set 3 : fix unit tests #Patch Set 4 : Add some more asserts :) #Patch Set 5 : testing button image height #Patch Set 6 : LabelButton -> ImageButton #Patch Set 7 : fix CrOS build #
Total comments: 5
Messages
Total messages: 39 (30 generated)
Description was changed from ========== BubbleFrameView: add top padding even when close button is hidden If there's no title or close button present, BubbleFrameView still needs some top padding to avoid the content being hard up against the top edge. Always pad as though the close button was visible, even if it's not, unless there's a title. BUG=622859 ========== to ========== BubbleFrameView: add top padding even when close button is hidden If there's no title or close button present, BubbleFrameView still needs some top padding to avoid the content being hard up against the top edge. Always pad as though the close button was visible, even if it's not, unless there's a title. BUG=622859 ==========
ellyjones@chromium.org changed reviewers: + msw@chromium.org
msw: ptal? :)
https://codereview.chromium.org/2148963002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2148963002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:248: close_->visible() || !has_title ? close_->height() : 0; Consider leaving close_height as-is and doing something like: const int min_height = ...; insets += gfx::Insets(std::max({title_height, close_height, min_height}), 0, 0, 0); Using ui/views/layout/layout_constants.h or close_->height() for min.
msw: ptal? :) https://codereview.chromium.org/2148963002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2148963002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:248: close_->visible() || !has_title ? close_->height() : 0; On 2016/07/13 19:23:44, msw wrote: > Consider leaving close_height as-is and doing something like: > const int min_height = ...; > insets += gfx::Insets(std::max({title_height, close_height, min_height}), 0, > 0, 0); > Using ui/views/layout/layout_constants.h or close_->height() for min. Done.
The CQ bit was checked by ellyjones@chromium.org 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...
hmm, it's still not ideal/obvious, but lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org 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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by ellyjones@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org 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 ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2148963002/#ps120001 (title: "fix CrOS build")
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 ========== BubbleFrameView: add top padding even when close button is hidden If there's no title or close button present, BubbleFrameView still needs some top padding to avoid the content being hard up against the top edge. Always pad as though the close button was visible, even if it's not, unless there's a title. BUG=622859 ========== to ========== BubbleFrameView: add top padding even when close button is hidden If there's no title or close button present, BubbleFrameView still needs some top padding to avoid the content being hard up against the top edge. Always pad as though the close button was visible, even if it's not, unless there's a title. BUG=622859 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== BubbleFrameView: add top padding even when close button is hidden If there's no title or close button present, BubbleFrameView still needs some top padding to avoid the content being hard up against the top edge. Always pad as though the close button was visible, even if it's not, unless there's a title. BUG=622859 ========== to ========== BubbleFrameView: add top padding even when close button is hidden If there's no title or close button present, BubbleFrameView still needs some top padding to avoid the content being hard up against the top edge. Always pad as though the close button was visible, even if it's not, unless there's a title. BUG=622859 Committed: https://crrev.com/f06ef4ac6ac3dd2f3a76486d2efde9ec35583ae6 Cr-Commit-Position: refs/heads/master@{#406856} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f06ef4ac6ac3dd2f3a76486d2efde9ec35583ae6 Cr-Commit-Position: refs/heads/master@{#406856}
Message was sent while issue was closed.
some nits for optional followup; generally still lgtm https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:26: #include "ui/views/controls/button/label_button.h" nit: remove https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:13: #include "ui/views/controls/button/image_button.h" optional nit: forward declare here; include header in the cc. https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:23: class LabelButton; nit: remove https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view_unittest.cc:124: int close_y = frame.GetCloseButtonForTest()->height(); nit: close_height https://codereview.chromium.org/2148963002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view_unittest.cc:444: ImageButton* button = frame.GetCloseButtonForTest(); nit: inline below.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2177533002/ by ellyjones@chromium.org. The reason for reverting is: Caused https://crbug.com/630539. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
