Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2959363002:
Add ripple effect to new-style notification header. (Closed)
DescriptionAdd ripple effect to new-style notification header.
BUG=736996
TEST=manual
Review-Url: https://codereview.chromium.org/2959363002
Cr-Commit-Position: refs/heads/master@{#483381}
Committed: https://chromium.googlesource.com/chromium/src/+/07abab334b42055ce5a277f78c78d39f584e5296
Patch Set 1 #
Total comments: 2
Patch Set 2 : Resolve review comments. #
Total comments: 10
Patch Set 3 : Resolve nits and remove focus from the header. #Patch Set 4 : Rebased. #
Messages
Total messages: 21 (13 generated)
The CQ bit was checked by tetsui@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...
tetsui@chromium.org changed reviewers: + fukino@chromium.org, yoshiki@chromium.org
PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2959363002/diff/1/ui/message_center/views/not... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2959363002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_header_view.cc:195: View::OnMouseEvent(event); This looks hacky (and scary)... In ash system menu (which uses HoverHighlightView), there is no color change on hover. Could you check how it works?
The CQ bit was checked by tetsui@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...
https://codereview.chromium.org/2959363002/diff/1/ui/message_center/views/not... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2959363002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_header_view.cc:195: View::OnMouseEvent(event); On 2017/06/29 04:30:42, fukino wrote: > This looks hacky (and scary)... > In ash system menu (which uses HoverHighlightView), there is no color change on > hover. > Could you check how it works? Done.
lgtm with nits https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:38: const float kInkDropRippleVisibleOpacity = 0.08f; nit: constexpr https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:40: const float kInkDropHighlightVisibleOpacity = 0.08f; ditto https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:173: std::unique_ptr<views::InkDropImpl> ink_drop = optional nit: You can use "auto" for remove the redundant the type names. It's optional and up to you. https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:190: std::unique_ptr<views::InkDropHighlight> highlight = ditto https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... File ui/message_center/views/notification_header_view.h (right): https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.h:35: std::unique_ptr<views::InkDrop> CreateInkDrop() override; nit: This class is mix of inherited and original methods. So please mention that these methods are overridden from a parent class. See the example of other classes. https://cs.chromium.org/search/?q=override:+file:.h$+package:%5Echromium$&typ...
looking good. Is it possible to trigger the ripple effect even when the expand button is clicked? It is a minor visual issue, so you can do it in a separate patch if it's going to be relatively big change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
yoshiki@: Thank you! fukino@: I realized that only forwarding ButtonPressed callback isn't enough, because ripple should start from press of the mouse button, not after the click with press and release action. So it needs subclassing of the ImageButton and I would do this in another CL. Thank you! https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:38: const float kInkDropRippleVisibleOpacity = 0.08f; On 2017/06/29 08:09:06, yoshiki wrote: > nit: constexpr Done. https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:40: const float kInkDropHighlightVisibleOpacity = 0.08f; On 2017/06/29 08:09:06, yoshiki wrote: > ditto Done. https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:173: std::unique_ptr<views::InkDropImpl> ink_drop = On 2017/06/29 08:09:06, yoshiki wrote: > optional nit: You can use "auto" for remove the redundant the type names. It's > optional and up to you. Done. https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:190: std::unique_ptr<views::InkDropHighlight> highlight = On 2017/06/29 08:09:06, yoshiki wrote: > ditto Done. https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... File ui/message_center/views/notification_header_view.h (right): https://codereview.chromium.org/2959363002/diff/20001/ui/message_center/views... ui/message_center/views/notification_header_view.h:35: std::unique_ptr<views::InkDrop> CreateInkDrop() override; On 2017/06/29 08:09:06, yoshiki wrote: > nit: This class is mix of inherited and original methods. So please mention that > these methods are overridden from a parent class. > > See the example of other classes. > https://cs.chromium.org/search/?q=override:+file:.h$+package:%5Echromium$&typ... Done.
The CQ bit was checked by tetsui@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org Link to the patchset: https://codereview.chromium.org/2959363002/#ps60001 (title: "Rebased.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498749451843400,
"parent_rev": "4455a5185e98db25d73eacaceb9f8713730197ec", "commit_rev":
"07abab334b42055ce5a277f78c78d39f584e5296"}
Message was sent while issue was closed.
Description was changed from ========== Add ripple effect to new-style notification header. BUG=736996 TEST=manual ========== to ========== Add ripple effect to new-style notification header. BUG=736996 TEST=manual Review-Url: https://codereview.chromium.org/2959363002 Cr-Commit-Position: refs/heads/master@{#483381} Committed: https://chromium.googlesource.com/chromium/src/+/07abab334b42055ce5a277f78c78... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/07abab334b42055ce5a277f78c78... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
