Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(810)

Issue 1242573005: Added the Material Design ink drop ripple effect to the navigation buttons in the browser toolbar. (Closed)

Created:
5 years, 5 months ago by bruthig
Modified:
5 years, 5 months ago
CC:
chromium-reviews, jonross, tdanderson, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Added the Material Design ink drop ripple effect to the navigation buttons in the browser toolbar. The ripple effect is behind the 'top-chrome-md' experimental switch. BUG=505587 Committed: https://crrev.com/6839695e30402d0ea6324a245a956807aeb5bdb7 Cr-Commit-Position: refs/heads/master@{#339650}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Addressed pkasting@'s comments from patch set 1. #

Total comments: 2

Patch Set 3 : Added comment as to why ripple was on ChromeOS only. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -0 lines) Patch
M chrome/browser/ui/views/toolbar/toolbar_button.h View 3 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/toolbar/toolbar_button.cc View 1 2 3 chunks +28 lines, -0 lines 1 comment Download

Messages

Total messages: 14 (4 generated)
bruthig
pkasting@, can you PTAL?
5 years, 5 months ago (2015-07-17 17:41:02 UTC) #2
Peter Kasting
https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/toolbar/toolbar_button.cc File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/toolbar/toolbar_button.cc#newcode34 chrome/browser/ui/views/toolbar/toolbar_button.cc:34: ink_drop_animation_controller_(nullptr), Omit this. https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/toolbar/toolbar_button.cc#newcode36 chrome/browser/ui/views/toolbar/toolbar_button.cc:36: #if defined(OS_CHROMEOS) Why is ...
5 years, 5 months ago (2015-07-17 18:01:58 UTC) #3
bruthig
pkasting@, can you PTAL? https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/toolbar/toolbar_button.cc File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/toolbar/toolbar_button.cc#newcode34 chrome/browser/ui/views/toolbar/toolbar_button.cc:34: ink_drop_animation_controller_(nullptr), On 2015/07/17 18:01:58, Peter ...
5 years, 5 months ago (2015-07-20 14:53:07 UTC) #4
Peter Kasting
lgtm https://codereview.chromium.org/1242573005/diff/20001/chrome/browser/ui/views/toolbar/toolbar_button.cc File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1242573005/diff/20001/chrome/browser/ui/views/toolbar/toolbar_button.cc#newcode35 chrome/browser/ui/views/toolbar/toolbar_button.cc:35: #if defined(OS_CHROMEOS) Can you at least add a ...
5 years, 5 months ago (2015-07-20 19:15:59 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1242573005/40001
5 years, 5 months ago (2015-07-21 13:41:00 UTC) #8
bruthig
https://codereview.chromium.org/1242573005/diff/20001/chrome/browser/ui/views/toolbar/toolbar_button.cc File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1242573005/diff/20001/chrome/browser/ui/views/toolbar/toolbar_button.cc#newcode35 chrome/browser/ui/views/toolbar/toolbar_button.cc:35: #if defined(OS_CHROMEOS) On 2015/07/20 19:15:59, Peter Kasting wrote: > ...
5 years, 5 months ago (2015-07-21 13:41:03 UTC) #9
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 5 months ago (2015-07-21 14:50:36 UTC) #10
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/6839695e30402d0ea6324a245a956807aeb5bdb7 Cr-Commit-Position: refs/heads/master@{#339650}
5 years, 5 months ago (2015-07-21 14:51:29 UTC) #11
Evan Stade
https://codereview.chromium.org/1242573005/diff/40001/chrome/browser/ui/views/toolbar/toolbar_button.cc File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/1242573005/diff/40001/chrome/browser/ui/views/toolbar/toolbar_button.cc#newcode174 chrome/browser/ui/views/toolbar/toolbar_button.cc:174: border->SetPainter(true, Button::STATE_PRESSED, nullptr); for my edification, is it intentional ...
5 years, 5 months ago (2015-07-22 16:29:46 UTC) #13
bruthig
5 years, 5 months ago (2015-07-23 14:28:14 UTC) #14
Message was sent while issue was closed.
On 2015/07/22 16:29:46, Evan Stade wrote:
>
https://codereview.chromium.org/1242573005/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):
> 
>
https://codereview.chromium.org/1242573005/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/toolbar/toolbar_button.cc:174:
border->SetPainter(true,
> Button::STATE_PRESSED, nullptr);
> for my edification, is it intentional that this also removes the pressed state
> for pointer events, even though the inkdrop only works for touch events? (Is
the
> inkdrop going to work with pointer events as well?)

Yes x2, this was intentional and the ink drop will eventually be activated for
pointer events as well as touch events.  Admittedly this does look a little
weird currently.

Powered by Google App Engine
This is Rietveld 408576698