Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10702102:
Added hover feedback on touch to launcher buttons. (Closed)
|
Created:
8 years, 5 months ago by Kevin Greer Modified:
8 years, 5 months ago CC:
chromium-reviews, sadrul, kuscher1, ben+watch_chromium.org Visibility:
Public. |
DescriptionAdded hover feedback on touch to launcher buttons.
BUG=131186
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148408
Patch Set 1 #Patch Set 2 : Removed stale include. #
Total comments: 7
Patch Set 3 : Fixed sorting issues. #Patch Set 4 : Fix for merge error. #Patch Set 5 : Merge with trunk #Messages
Total messages: 14 (0 generated)
Please review. There was some discussion in the ticket about if the delay should be 200 or 500ms, but the 200ms (the same used for hover) feels and looks good (to me).
https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... File ash/launcher/launcher_button.cc (right): https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... ash/launcher/launcher_button.cc:18: #include "ui/base/events.h" Sort https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... ash/launcher/launcher_button.cc:335: AddState(STATE_HOVERED); Since this class derives from CustomButton, I think we should make the hopping animation based on CustomButton's HOT state_. https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... File ash/launcher/launcher_button.h (right): https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... ash/launcher/launcher_button.h:86: OVERRIDE; In general I think we try to order these in the same order as the class they come from. In this case it would be after OnMouseExited.
Fixed sorting issues. https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... File ash/launcher/launcher_button.cc (right): https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... ash/launcher/launcher_button.cc:18: #include "ui/base/events.h" On 2012/07/05 14:11:12, flackr wrote: > Sort Done. https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... File ash/launcher/launcher_button.h (right): https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... ash/launcher/launcher_button.h:86: OVERRIDE; On 2012/07/05 14:11:12, flackr wrote: > In general I think we try to order these in the same order as the class they > come from. In this case it would be after OnMouseExited. Done.
Comment / "request for clarification" added. https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... File ash/launcher/launcher_button.cc (right): https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... ash/launcher/launcher_button.cc:335: AddState(STATE_HOVERED); LauncherButton creates its own new set of states that it uses instead of those defined by CustomButton. LauncherButton's HOT state is defined as 1, the same as CustomButton's HOVERED, so we can't really distinguish between them anyway. Are you sure it makes sense to mix & match states from different classes/enums? Can you please clarify what you have in mind? Thanks On 2012/07/05 14:11:12, flackr wrote: > Since this class derives from CustomButton, I think we should make the hopping > animation based on CustomButton's HOT state_.
https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... File ash/launcher/launcher_button.cc (right): https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... ash/launcher/launcher_button.cc:335: AddState(STATE_HOVERED); What I have in mind is to use CustomButton's tracking of the hover state. Currently we are duplicating the effort of determining if the button is in its hover state here even though CustomButton's state_ already tracks this. We should rename this class's state to something descriptive of the states which it would track, maybe launcher_state_? And then use the hover state from CustomButton and remove the code here since it is already done in CustomButton. On 2012/07/05 21:06:05, Kevin Greer wrote: > LauncherButton creates its own new set of states that it uses instead of those > defined by CustomButton. LauncherButton's HOT state is defined as 1, the same > as CustomButton's HOVERED, so we can't really distinguish between them anyway. > Are you sure it makes sense to mix & match states from different classes/enums? > Can you please clarify what you have in mind? Thanks > > On 2012/07/05 14:11:12, flackr wrote: > > Since this class derives from CustomButton, I think we should make the hopping > > animation based on CustomButton's HOT state_. >
I think what flackr describes is fine but I wouldn't block this cl on the change. Adding this code doesn't make the current situation any worse and would be straightforward to change if / when we do as he describes. On 2012/07/06 03:12:58, flackr wrote: > https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... > File ash/launcher/launcher_button.cc (right): > > https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... > ash/launcher/launcher_button.cc:335: AddState(STATE_HOVERED); > What I have in mind is to use CustomButton's tracking of the hover state. > Currently we are duplicating the effort of determining if the button is in its > hover state here even though CustomButton's state_ already tracks this. We > should rename this class's state to something descriptive of the states which it > would track, maybe launcher_state_? And then use the hover state from > CustomButton and remove the code here since it is already done in CustomButton. > > On 2012/07/05 21:06:05, Kevin Greer wrote: > > LauncherButton creates its own new set of states that it uses instead of those > > defined by CustomButton. LauncherButton's HOT state is defined as 1, the same > > as CustomButton's HOVERED, so we can't really distinguish between them anyway. > > > Are you sure it makes sense to mix & match states from different > classes/enums? > > Can you please clarify what you have in mind? Thanks > > > > On 2012/07/05 14:11:12, flackr wrote: > > > Since this class derives from CustomButton, I think we should make the > hopping > > > animation based on CustomButton's HOT state_. > >
Sure, LGTM otherwise. On 2012/07/24 14:47:54, DaveMoore wrote: > I think what flackr describes is fine but I wouldn't block this cl on the > change. Adding this code doesn't make the current situation any worse and would > be straightforward to change if / when we do as he describes. > > On 2012/07/06 03:12:58, flackr wrote: > > > https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... > > File ash/launcher/launcher_button.cc (right): > > > > > https://chromiumcodereview.appspot.com/10702102/diff/2001/ash/launcher/launch... > > ash/launcher/launcher_button.cc:335: AddState(STATE_HOVERED); > > What I have in mind is to use CustomButton's tracking of the hover state. > > Currently we are duplicating the effort of determining if the button is in its > > hover state here even though CustomButton's state_ already tracks this. We > > should rename this class's state to something descriptive of the states which > it > > would track, maybe launcher_state_? And then use the hover state from > > CustomButton and remove the code here since it is already done in > CustomButton. > > > > On 2012/07/05 21:06:05, Kevin Greer wrote: > > > LauncherButton creates its own new set of states that it uses instead of > those > > > defined by CustomButton. LauncherButton's HOT state is defined as 1, the > same > > > as CustomButton's HOVERED, so we can't really distinguish between them > anyway. > > > > > Are you sure it makes sense to mix & match states from different > > classes/enums? > > > Can you please clarify what you have in mind? Thanks > > > > > > On 2012/07/05 14:11:12, flackr wrote: > > > > Since this class derives from CustomButton, I think we should make the > > hopping > > > > animation based on CustomButton's HOT state_. > > >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kgr@chromium.org/10702102/6001
Failed to apply patch for ash/launcher/launcher_button.cc: While running patch -p1 --forward --force; patching file ash/launcher/launcher_button.cc Hunk #1 FAILED at 10. Hunk #2 succeeded at 380 with fuzz 1 (offset 15 lines). 1 out of 2 hunks FAILED -- saving rejects to file ash/launcher/launcher_button.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kgr@chromium.org/10702102/15001
Failed to apply patch for ash/launcher/launcher_button.cc: While running patch -p1 --forward --force; patching file ash/launcher/launcher_button.cc Hunk #1 FAILED at 10. Hunk #2 succeeded at 380 with fuzz 1 (offset 15 lines). 1 out of 2 hunks FAILED -- saving rejects to file ash/launcher/launcher_button.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kgr@chromium.org/10702102/19001
Change committed as 148408 |
