Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579463002:
Fix layout of launcher and overflow buttons in Ash MD shelf (Closed)
|
Created:
4 years ago by tdanderson Modified:
3 years, 11 months ago Reviewers:
oshima CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix layout of launcher and overflow buttons in Ash MD shelf
This CL ensures the correct layout of the launcher
button and the overflow button in the Ash MD shelf
when the shelf is being resized. This can occur in
two situations: when the shelf is being 'stretched'
toward the center of the screen by touch, or
during a shelf hide/show animation.
BUG=668230
TEST=manual
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments #Patch Set 3 : restructuring #
Messages
Total messages: 19 (10 generated)
The CQ bit was checked by tdanderson@chromium.org to run a CQ dry run
tdanderson@chromium.org changed reviewers: + oshima@chromium.org
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.
https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... ash/common/shelf/app_list_button.cc:298: // adjust the x-position for a left- or right-aligned shelf. am I correct that we assume the button is square? https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/overflow_b... File ash/common/shelf/overflow_button.cc (right): https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/overflow_b... ash/common/shelf/overflow_button.cc:175: ResourceBundle& rb = ResourceBundle::GetSharedInstance(); unrelated, but you can move this to else block. https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/overflow_b... ash/common/shelf/overflow_button.cc:176: if (MaterialDesignController::IsShelfMaterial()) { just add quick comment saying "align to the top"
The CQ bit was checked by tdanderson@chromium.org to run a CQ dry run
Please take another look https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... ash/common/shelf/app_list_button.cc:298: // adjust the x-position for a left- or right-aligned shelf. On 2016/12/14 21:28:41, oshima wrote: > am I correct that we assume the button is square? The app list button is actually a circle https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/overflow_b... File ash/common/shelf/overflow_button.cc (right): https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/overflow_b... ash/common/shelf/overflow_button.cc:175: ResourceBundle& rb = ResourceBundle::GetSharedInstance(); On 2016/12/14 21:28:41, oshima wrote: > unrelated, but you can move this to else block. Done. https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/overflow_b... ash/common/shelf/overflow_button.cc:176: if (MaterialDesignController::IsShelfMaterial()) { On 2016/12/14 21:28:41, oshima wrote: > just add quick comment saying "align to the top" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... ash/common/shelf/app_list_button.cc:298: // adjust the x-position for a left- or right-aligned shelf. On 2016/12/14 21:45:07, tdanderson wrote: > On 2016/12/14 21:28:41, oshima wrote: > > am I correct that we assume the button is square? > > The app list button is actually a circle I meant the logical bounds of the button is square(-ish). optional: following may be more readable. up to you. if (bottom) { return gfx::Point(x_mid, x_mid); } else if (right) { return gfx::POint(x_mid, y_mid); } else { DCHECK_(left); return gfx::POint(width() - y_mid, x_mid); }
https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2579463002/diff/1/ash/common/shelf/app_list_b... ash/common/shelf/app_list_button.cc:298: // adjust the x-position for a left- or right-aligned shelf. On 2016/12/14 21:52:37, oshima wrote: > On 2016/12/14 21:45:07, tdanderson wrote: > > On 2016/12/14 21:28:41, oshima wrote: > > > am I correct that we assume the button is square? > > > > The app list button is actually a circle > > I meant the logical bounds of the button is square(-ish). > optional: following may be more readable. up to you. > > if (bottom) { > return gfx::Point(x_mid, x_mid); > } else if (right) { > return gfx::POint(x_mid, y_mid); > } else { > DCHECK_(left); > return gfx::POint(width() - y_mid, x_mid); > } Thanks, I like that much better. Changed.
The CQ bit was checked by tdanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2579463002/#ps40001 (title: "restructuring")
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
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_...)
On 2016/12/14 23:06:46, commit-bot: I haz the power wrote: > 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_...) Trying to fix test failures with: https://codereview.chromium.org/2598223002/
On 2016/12/23 20:34:18, bruthig wrote: > On 2016/12/14 23:06:46, commit-bot: I haz the power wrote: > > 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_...) > > Trying to fix test failures with: https://codereview.chromium.org/2598223002/ Closing this, it was addressed by the CL here: https://codereview.chromium.org/2598223002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
