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
Add hover duration for Inkwell
widget
#132176
Changes from 3 commits
f788714
f05e93a
024ba90
c6720e8
ace8996
ccb62f5
780da4e
f31ff84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -332,6 +332,7 @@ class InkResponse extends StatelessWidget { | |
this.onFocusChange, | ||
this.autofocus = false, | ||
this.statesController, | ||
this.hoverDuration = const Duration(milliseconds: 50), | ||
}); | ||
|
||
/// The widget below this widget in the tree. | ||
|
@@ -621,6 +622,9 @@ class InkResponse extends StatelessWidget { | |
/// {@endtemplate} | ||
final MaterialStatesController? statesController; | ||
|
||
/// The duration of the animation that animates the hover effect. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Maybe state that the default is 50 milliseconds.
HansMuller marked this conversation as resolved.
Show resolved
Hide resolved
|
||
final Duration hoverDuration; | ||
|
||
@override | ||
Widget build(BuildContext context) { | ||
final _ParentInkResponseState? parentState = _ParentInkResponseProvider.maybeOf(context); | ||
|
@@ -659,6 +663,7 @@ class InkResponse extends StatelessWidget { | |
getRectCallback: getRectCallback, | ||
debugCheckContext: debugCheckContext, | ||
statesController: statesController, | ||
hoverDuration: hoverDuration, | ||
child: child, | ||
); | ||
} | ||
|
@@ -715,6 +720,7 @@ class _InkResponseStateWidget extends StatefulWidget { | |
this.getRectCallback, | ||
required this.debugCheckContext, | ||
this.statesController, | ||
this.hoverDuration = const Duration(milliseconds: 50), | ||
}); | ||
|
||
final Widget? child; | ||
|
@@ -752,6 +758,7 @@ class _InkResponseStateWidget extends StatefulWidget { | |
final _GetRectCallback? getRectCallback; | ||
final _CheckContext debugCheckContext; | ||
final MaterialStatesController? statesController; | ||
final Duration hoverDuration; | ||
|
||
@override | ||
_InkResponseState createState() => _InkResponseState(); | ||
|
@@ -920,7 +927,7 @@ class _InkResponseState extends State<_InkResponseStateWidget> | |
return const Duration(milliseconds: 200); | ||
case _HighlightType.hover: | ||
case _HighlightType.focus: | ||
return const Duration(milliseconds: 50); | ||
return widget.hoverDuration; | ||
} | ||
} | ||
|
||
|
@@ -1456,6 +1463,7 @@ class InkWell extends InkResponse { | |
super.onFocusChange, | ||
super.autofocus, | ||
super.statesController, | ||
super.hoverDuration, | ||
}) : super( | ||
containedInkWell: true, | ||
highlightShape: BoxShape.rectangle, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2230,4 +2230,28 @@ testWidgetsWithLeakTracking('InkResponse radius can be updated', (WidgetTester t | |
|
||
await gesture.up(); | ||
}); | ||
|
||
testWidgetsWithLeakTracking('try out hoverDuration property', (WidgetTester tester) async { | ||
final List<String> log = <String>[]; | ||
|
||
await tester.pumpWidget(Directionality( | ||
textDirection: TextDirection.ltr, | ||
child: Material( | ||
child: Center( | ||
child: InkWell( | ||
hoverDuration: const Duration(milliseconds: 1000), | ||
onTap: () { | ||
log.add('tap'); | ||
}, | ||
), | ||
), | ||
), | ||
)); | ||
|
||
await tester.tap(find.byType(InkWell), pointer: 1); | ||
await tester.pump(const Duration(seconds: 1)); | ||
|
||
expect(log, equals(<String>['tap'])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain how this test works? Would it actually fail without the change in this PR? I expected it to test a color or something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test would fail without this pr because it's making use of the new |
||
log.clear(); | ||
}); | ||
} |
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.
Prefer to make this default to null and introduce the default with
hoverDuration ?? const Duration(milliseconds: 50)
. That will make it easier to override the default via a theme later.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.
Done