Add enableFeedback param to MaterialButton, RawMaterialButton and IconButton #41972

NikitaZhelonkin · 2019-10-04T10:26:23Z

In some cases developers need to disable default tap feedback, e.g. if you want to implement custom feedback. This PR gives that opportunity

fluttergithubbot · 2019-10-04T10:26:26Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

googlebot · 2019-10-04T10:26:27Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

NikitaZhelonkin · 2019-10-04T10:28:38Z

@googlebot I signed it!

googlebot · 2019-10-04T10:28:40Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

shihaohong · 2019-10-04T15:17:34Z

Hi, this change looks good! Could you add a few tests to verify that the enableFeedback parameter is working properly for MaterialButton and RawMaterialButton? There should be some useful pointers in ink_well_test.dart.

Thank you!

NikitaZhelonkin · 2019-10-05T06:43:08Z

How can i fix " Flutter build is currently broken" ?

shihaohong · 2019-10-07T04:30:36Z

That isn't an issue with your PR, so there's nothing you need to do about it.

That basically checks if Flutter's latest master is failing for whatever reason. This prevents new changes from being introduced if there is a potential existing issue in the codebase.

shihaohong

Looks pretty good! I have a request about adding an additional test case, and we should wait to see what Hans has to say about handling long presses with material buttons.

shihaohong · 2019-10-07T17:14:31Z

packages/flutter/lib/src/material/icon_button.dart

+  /// Whether detected gestures should provide acoustic and/or haptic feedback.
+  ///
+  /// For example, on Android a tap will produce a clicking sound and a
+  /// long-press will produce a short vibration, when feedback is enabled.


It seems that RawMaterialButton, MaterialButton and IconButton do not have an onLongPress callback or option, which makes this inaccurate. We should either modify the docs to explain that long-press is not supported for these widgets, or implement them correctly.

@HansMuller What seems like the appropriate step here?

Related to this issue somewhat, since it explains the default long press behavior for Material buttons and the intent to add an onLongPress param: #40641

Removed comment in doc about long-press

I don't think just removing it is the right approach -- I think if we take the route of not having it respond to long presses, we need to explain how this is different from what is expected. Let's wait for @HansMuller before we continue forward about this topic

I think that #40641 will land pretty soon and so it's OK to explain that enableFeedback does what InkWell.enableFeedback does without qualification.

shihaohong · 2019-10-07T17:17:16Z

packages/flutter/lib/src/material/material_button.dart

+  /// Whether detected gestures should provide acoustic and/or haptic feedback.
+  ///
+  /// For example, on Android a tap will produce a clicking sound and a
+  /// long-press will produce a short vibration, when feedback is enabled.


Same comment regarding long-press

Removed comment in doc about long-press

Commenting here that we've re-added the comment since long-press is now enabled on these buttons

shihaohong · 2019-10-07T17:22:09Z

packages/flutter/test/material/buttons_test.dart

@@ -1191,4 +1192,45 @@ void main() {
    final Material material = tester.widget<Material>(rawButtonMaterial);
    expect(material.color, const Color(0xff00ff00));
  });
+
+  group('feedback', () {


Could you add a set of these tests for RawMaterialButton as well?

shihaohong · 2019-10-07T17:22:23Z

packages/flutter/test/material/buttons_test.dart

+      expect(feedback.hapticCount, 0);
+    });
+  });
+


nit: remove these two additional newlines

shihaohong · 2019-10-07T17:22:36Z

packages/flutter/test/material/icon_button_test.dart

@@ -397,6 +398,56 @@ void main() {
    expect(focusNode1.hasPrimaryFocus, isTrue);
    expect(focusNode2.hasPrimaryFocus, isFalse);
  });
+


nit: remove one newline, since two were added

shihaohong · 2019-10-07T17:24:54Z

packages/flutter/test/material/buttons_test.dart

+      expect(feedback.hapticCount, 0);
+    });
+
+    testWidgets('MaterialButton with enabled feedback', (WidgetTester tester) async {


This feels like it tests the default behavior (that enableFeedback is set to true by default). You should probably add a test where enableFeedback is explicitly set to true. ie.

await tester.pumpWidget(Directionality( textDirection: TextDirection.ltr, child: MaterialButton( onPressed: () {}, enableFeedback: true, // add a test for this case ), ));

shihaohong · 2019-10-07T17:26:18Z

packages/flutter/test/material/icon_button_test.dart

+      expect(feedback.hapticCount, 0);
+    });
+
+    testWidgets('IconButton with enabled feedback', (WidgetTester tester) async {


Same comment about testing for explicitly setting enableFeedback: true.

…n and IconButton

…d IconButton

shihaohong

The rest of the changes look good, let's just wait to hear back about the long press detail :)

… and IconButton

shihaohong · 2019-10-30T01:52:25Z

packages/flutter/lib/src/material/icon_button.dart

-          (iconSize + math.min(padding.horizontal, padding.vertical)) * 0.7,
-          // x 0.5 for diameter -> radius and + 40% overflow derived from other Material apps.
+        enableFeedback: enableFeedback,
+          child: result,


One of the merges with master cluttered up this section of code. It should be the original, adding the enableFeedback param.

shihaohong · 2019-10-30T01:53:22Z

packages/flutter/lib/src/material/material_button.dart

+  /// Whether detected gestures should provide acoustic and/or haptic feedback.
+  ///
+  /// For example, on Android a tap will produce a clicking sound and a
+  /// long-press will produce a short vibration, when feedback is enabled.


Commenting here that we've re-added the comment since long-press is now enabled on these buttons

shihaohong · 2019-10-30T17:57:03Z

@NikitaZhelonkin do you have the bandwidth to finish up this PR?

NikitaZhelonkin · 2019-10-30T18:10:11Z

Yes, what I need to do?

shihaohong · 2019-10-30T18:28:49Z

There are some failing pre-submit tests that need to be resolved. Also, there is a part of the code I commented on which was formatted incorrectly in one of the recent merges to master that needs to be fixed.

NikitaZhelonkin · 2019-10-31T13:44:21Z

packages/flutter/test/material/buttons_test.dart

-  });
-
-  group('feedback', () {
-    FeedbackTester feedback;


@shihaohong Why did you remove those tests?

It was causing a merge conflict since it's part of an ongoing effort to remove the buttons_test.dart file. There are more specific files that some of these tests belong to (ie. flat_button_test.dart), and it made more sense to have them there.

However, if I seem to have removed a test accidentally that wasn't intentional, please let me know (on looking at the files changed tab on GitHub, it seems like everything looks okay).

shihaohong · 2019-11-01T16:27:31Z

Hi, any updates on this PR? I just want to make sure we can get this one moving along since it's so close to the finish line!

NikitaZhelonkin · 2019-11-01T16:40:48Z

Fixed formatting; Seems some checks don't pass after merge master from origin repository

shihaohong · 2019-11-01T16:57:18Z

It seems to be failing the following test:

   info • Unused import: 'feedback_tester.dart' • packages/flutter/test/material/buttons_test.dart:13:8 • unused_import

shihaohong · 2019-11-01T16:57:43Z

packages/flutter/test/material/buttons_test.dart

@@ -10,6 +10,7 @@ import 'package:flutter_test/flutter_test.dart';

 import '../rendering/mock_canvas.dart';
 import '../widgets/semantics_tester.dart';
+import 'feedback_tester.dart';


Remove this line

shihaohong · 2019-11-01T18:01:32Z

Make sure to run flutter analyze --flutter-repo locally after your changes to make sure that all analyzer issues are properly addressed.

▌17:09:49▐ RUNNING: cd .; grep --line-number --extended-regexp [[:blank:]]$ packages/flutter/lib/src/material/button.dart packages/flutter/lib/src/material/icon_button.dart packages/flutter/lib/src/material/material_button.dart packages/flutter/test/material/icon_button_test.dart
packages/flutter/lib/src/material/material_button.dart:377:      enableFeedback: enableFeedback,      
▌17:09:49▐ ELAPSED TIME: 0.025s for grep --line-number --extended-regexp [[:blank:]]$ packages/flutter/lib/src/material/button.dart packages/flutter/lib/src/material/icon_button.dart packages/flutter/lib/src/material/material_button.dart packages/flutter/test/material/icon_button_test.dart in .
Detected trailing whitespace in the file[s] listed above.

NikitaZhelonkin · 2019-11-01T19:05:35Z

It shows '36816 issues found." if i run it locally

packages/flutter/lib/src/material/material_button.dart

shihaohong · 2019-11-01T19:11:38Z

Is there any more detail as to what the issues are? Make sure that you've run flutter update-packages. It might be that your local copy of Flutter's dependencies are not available.

Co-Authored-By: Shi-Hao Hong <shihaohong@google.com>

NikitaZhelonkin · 2019-11-01T19:22:18Z

Yes, flutter update-packages helped, thank you!

shihaohong

LGTM

…nButton (flutter#41972) * Wire enableFeedback parameter through MaterialButton, RawMaterialButton, and IconButton. Co-Authored-By: Shi-Hao Hong <shihaohong@google.com>

Add enableFeedback param to MaterialButton

fda90e2

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 4, 2019

googlebot added the cla: no label Oct 4, 2019

googlebot added cla: yes and removed cla: no labels Oct 4, 2019

shihaohong added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Oct 4, 2019

shihaohong changed the title ~~Add enableFeedback param to MaterialButton~~ Add enableFeedback param to MaterialButton and RawMaterialButton Oct 4, 2019

NikitaZhelonkin added 2 commits October 5, 2019 09:37

Test MaterialButton enableFeedback

7612069

Add enableFeedback param to IconButton

3fab0b8

NikitaZhelonkin changed the title ~~Add enableFeedback param to MaterialButton and RawMaterialButton~~ Add enableFeedback param to MaterialButton, RawMaterialButton and IconButton Oct 5, 2019

shihaohong removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Oct 7, 2019

shihaohong reviewed Oct 7, 2019

View reviewed changes

shihaohong self-assigned this Oct 7, 2019

NikitaZhelonkin added 3 commits October 8, 2019 09:31

Fix enableFeedback documentation for MaterialButton, RawMaterialButto…

2258210

…n and IconButton

Add more enableFeedback test for MaterialButton, RawMaterialButton an…

2076918

…d IconButton

Add line

1039826

shihaohong reviewed Oct 8, 2019

View reviewed changes

NikitaZhelonkin added 2 commits October 15, 2019 10:30

Fix doc for enableFeedback param in MaterialButton, RawMaterialButton…

c2738ac

… and IconButton

Merge branch 'master' of https://github.com/flutter/flutter

456a225

shihaohong added the waiting for PR to land (fixed) A fix is in flight label Oct 15, 2019

NikitaZhelonkin and others added 2 commits October 23, 2019 10:12

Merge branch 'master' of https://github.com/flutter/flutter

22be52b

Merge branch 'master' into master

2038c9e

shihaohong reviewed Oct 30, 2019

View reviewed changes

NikitaZhelonkin commented Oct 31, 2019

View reviewed changes

shihaohong added waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds and removed waiting for PR to land (fixed) A fix is in flight labels Nov 1, 2019

Fix formatting

38d1db3

Merge branch 'master' of https://github.com/flutter/flutter

d9ee1bc

shihaohong reviewed Nov 1, 2019

View reviewed changes

Remove unused import

36dafff

shihaohong reviewed Nov 1, 2019

View reviewed changes

packages/flutter/lib/src/material/material_button.dart Outdated Show resolved Hide resolved

Remove whitespaces

d0362bc

Co-Authored-By: Shi-Hao Hong <shihaohong@google.com>

shihaohong approved these changes Nov 1, 2019

View reviewed changes

shihaohong merged commit fbabb26 into flutter:master Nov 1, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Add enableFeedback param to MaterialButton, RawMaterialButton and IconButton #41972

Add enableFeedback param to MaterialButton, RawMaterialButton and IconButton #41972

Conversation

NikitaZhelonkin commented Oct 4, 2019 • edited

fluttergithubbot commented Oct 4, 2019

googlebot commented Oct 4, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

NikitaZhelonkin commented Oct 4, 2019

googlebot commented Oct 4, 2019

shihaohong commented Oct 4, 2019

NikitaZhelonkin commented Oct 5, 2019

shihaohong commented Oct 7, 2019 • edited

shihaohong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihaohong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihaohong commented Oct 30, 2019

NikitaZhelonkin commented Oct 30, 2019

shihaohong commented Oct 30, 2019

Choose a reason for hiding this comment

shihaohong Oct 31, 2019 • edited

Choose a reason for hiding this comment

shihaohong commented Nov 1, 2019

NikitaZhelonkin commented Nov 1, 2019 • edited

shihaohong commented Nov 1, 2019

Choose a reason for hiding this comment

shihaohong commented Nov 1, 2019

NikitaZhelonkin commented Nov 1, 2019

shihaohong commented Nov 1, 2019

NikitaZhelonkin commented Nov 1, 2019

shihaohong left a comment

Choose a reason for hiding this comment

NikitaZhelonkin commented Oct 4, 2019 •

edited

shihaohong commented Oct 7, 2019 •

edited

shihaohong Oct 31, 2019 •

edited

NikitaZhelonkin commented Nov 1, 2019 •

edited