-
Notifications
You must be signed in to change notification settings - Fork 361
button: Replace GestureDetector with Listener for AnimatedScaleOnTap
#1954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fixes zulip#1953. Latter provides instant scaleDown animation on tap, unlike GestureDetector which has a `kPressTimeOut` causing delay of 100ms https://main-api.flutter.dev/flutter/gestures/kPressTimeout-constant.html
7710b5d to
7034690
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Comments below.
| check(scale).equals(expectedScale); | ||
| } | ||
|
|
||
| testWidgets('Animation happen instantly when tap down', (tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this test without your change in lib/widgets/button.dart, it still passes. That means it's not doing its job.
| return Listener( | ||
| behavior: HitTestBehavior.translucent, | ||
| onTapDown: (_) => _changeScale(widget.scaleEnd), | ||
| onTapUp: (_) => _changeScale(1), | ||
| onTapCancel: () => _changeScale(1), | ||
| onPointerDown: (_) => _changeScale(widget.scaleEnd), | ||
| onPointerUp: (_) => _changeScale(1), | ||
| onPointerCancel: (_) => _changeScale(1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the docs for GestureDetector.onTapDown and friends, it looks like one of the helpful features of that higher-level API is that it only considers pointer events from a "primary button". I don't know how common it is e.g. to use a stylus with multiple buttons, but this seems like functionality that would be helpful to keep.
The event passed to the Listener.onPointerDown callback (etc.) has a buttons field. So in each of the handlers, let's check whether the event's buttons field equals kPrimaryButton, and only respond if it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the name and dartdoc of this AnimatedScaleOnTap should be updated to match the new behavior: it's no longer about a tap gesture, which is only recognized after 100ms. For example:
/// Apply [Transform.scale] to the child widget on primary pointer-down,
/// and reset its scale on -up or -cancel, with animated transitions.
class AnimatedScaleOnPointerDown extends StatefulWidget {
Fixes #1953.
The Scale down and Scale up as well as Ink Splash animation occurs after a finite delay of 100ms. Mainly due to
GestureDetectorused insideAnimatedScaleOnTap.This can be fixed by using
Listenerhence replacing theGestureDetectorinsideAnimatedScaleOnTap.Discussion: #mobile > issue in responsiveness of home bottom navbar icons
Design: Figma Design
Comparison Through Video of Fix-up
Before
implementation.mp4
After
fixup.mp4