Add searchFieldLabel to SearchDelegate in order to show a custom hint #36409

knaeckeKami · 2019-07-18T00:33:44Z

Description

This fixes issue #18831. A new, optional member "searchFieldLabel" of SearchDelegate was added to specify the label of the search field. If it's specified, it it displayed instead of the default value from DefaultMaterialLocalizations.

Credits to @somarkoe for preparing the addition of some of the added features in #28807

Related Issues

#18831

Tests

I added the following tests:

"Showing custom search hint": a widget test that verifies that the custom search field label is indeed displayed

"Showing default search label if no custom label is given": a widget test that verifies if no searchFieldLabel is specified, the current behaviour of looking up the value from DefaultMaterialLocalizations is unchanged.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

… label.

shihaohong

I have a few code formatting and API doc suggestions, but otherwise, LGTM

shihaohong · 2019-07-24T00:19:02Z

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

    assert(debugCheckHasMaterialLocalizations(context));
    final ThemeData theme = widget.delegate.appBarTheme(context);
-    final String searchFieldLabel = MaterialLocalizations.of(context).searchFieldLabel;
+    final String searchFieldLabel = widget.delegate.searchFieldLabel ??


I think this is more readable:

final String searchFieldLabel = widget.delegate.searchFieldLabel ?? MaterialLocalizations.of(context).searchFieldLabel;

shihaohong · 2019-07-24T00:22:06Z

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

    _queryTextController.text = value;
  }

+  /// The hint text to be shown when the search field is empty.


Suggested change

/// The hint text to be shown when the search field is empty.

/// The hint text that is shown in the search field when it is empty.

Add some documentation to describe what the hint text defaults to if this value is null in another line:
If this value is set to null, the value of MaterialLocalizations.of(context).searchFieldLabel will be used instead.

Also, maybe a code sample for how to use this (as you did with the _TestSearchDelegate in the tests) for other developers.

shihaohong · 2019-07-24T00:23:29Z

packages/flutter/test/material/search_test.dart

    expect(selectedResults, <String>['Result Foo']);
  });

+  testWidgets('Showing custom search hint', (WidgetTester tester) async {


Suggested change

testWidgets('Showing custom search hint', (WidgetTester tester) async {

testWidgets('Custom searchFieldLabel value', (WidgetTester tester) async {

shihaohong · 2019-07-24T00:24:07Z

packages/flutter/test/material/search_test.dart

+  testWidgets('Showing custom search hint', (WidgetTester tester) async {
+    const String searchHint = 'custom search hint';
+    final String defaultSearchHint =
+        const DefaultMaterialLocalizations().searchFieldLabel;


This can be in the same line as the previous line

shihaohong · 2019-07-24T00:24:10Z

packages/flutter/test/material/search_test.dart

+        const DefaultMaterialLocalizations().searchFieldLabel;
+
+    final _TestSearchDelegate delegate =
+        _TestSearchDelegate(searchHint: searchHint);


This can be in the same line as the previous line

shihaohong · 2019-07-24T00:24:48Z

packages/flutter/test/material/search_test.dart

+    expect(find.text(defaultSearchHint), findsNothing);
+  });
+
+  testWidgets('Showing default search label if no custom label is given', (WidgetTester tester) async {


Suggested change

testWidgets('Showing default search label if no custom label is given', (WidgetTester tester) async {

testWidgets('Default searchFieldLabel is used when it is set to null', (WidgetTester tester) async {

shihaohong · 2019-07-24T00:25:19Z

packages/flutter/test/material/search_test.dart

+
+  testWidgets('Showing default search label if no custom label is given', (WidgetTester tester) async {
+    final String searchHint =
+        const DefaultMaterialLocalizations().searchFieldLabel;


This can be in the same line as the previous line

knaeckeKami · 2019-07-24T10:07:52Z

Thank you for the review, I applied your suggestions.

shihaohong · 2019-07-24T15:39:21Z

You missed one of the suggestions (search_test.dart L487). Also, you might have to merge your branch with master, one of the presubmit tests are failing, but it seems unlikely to be from your change.

shihaohong · 2019-07-24T15:40:14Z

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

+  ///   @override
+  ///   String get searchFieldLabel => this.searchHint;
+  ///
+  ///    @override


Suggested change

/// @override

/// @override

shihaohong · 2019-07-24T15:40:57Z

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

+  ///
+  ///   final String searchHint;
+  ///
+  ///   CustomSearchHintDelegate({


The constructor should typically be above every other member/method, so just move this above searchHint

shihaohong · 2019-07-24T15:41:47Z

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

+  ///
+  /// ```dart
+  /// class CustomSearchHintDelegate extends SearchDelegate {
+  ///


No need for newline here

knaeckeKami · 2019-07-24T16:15:49Z

Yeah, I don't see why my changes would have caused problems specifically on windows. I merged with master and applied the rest of your suggestions.

shihaohong · 2019-07-24T17:26:45Z

It was brought to my attention that this change is related to #28807, except they have some additional functionality, such as keyboard type and the input action.

While I don't necessarily think this should stop your PR if you don't want to include those features, it might be worth looking over some of their comments/discussion and seeing if there is something there that you can apply here, notably suggestions like the one in this comment.

…Delegate

knaeckeKami · 2019-07-24T19:42:32Z

I don't mind adding support for textInputAction and keyboardType. I hope it's not considered rude to "steal" features from another PR, but it seems @somarkoe isn't very responsive in his PR at the moment.

I made searchFieldLabel, keyboardType, and textInputAction final fields the can be set in the constructor. I also find this approach cleaner, albeit maybe little bit less discoverable. I tried to compensate for this by moving the code sample to the constructor.

shihaohong

This looks great! Thanks for doing the additional work of adding the new properties in.

Just a few minor styling suggestions. LGTM

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

packages/flutter/test/material/search_test.dart

shihaohong · 2019-07-24T19:52:24Z

I don't mind adding support for textInputAction and keyboardType. I hope it's not considered rude to "steal" features from another PR, but it seems @somarkoe isn't very responsive in his PR at the moment.

I made searchFieldLabel, keyboardType, and textInputAction final fields the can be set in the constructor. I also find this approach cleaner, albeit maybe little bit less discoverable. I tried to compensate for this by moving the code sample to the constructor.

One thing I would definitely do is give @somarkoe credit in the first comment of this PR, something along the lines of this PR.

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

samlythemanly · 2019-07-24T19:59:01Z

I hope it's not considered rude to "steal" features from another PR

Not rude at all! I actually quite appreciate it.

I made the initial PR when I had a endless amounts of free time back in March, but once it actually was pulled into the review process I was swamped in both personal life events as well as work responsibilities and that hasn't really changed!

knaeckeKami · 2019-07-24T20:19:54Z

Applied all suggestions.

…flutter#36409) * Add searchFieldLabel to SearchDelegate in order to show a custom hint label. * Add support for specifying textInputAction and keyboardType in SearchDelegate

Martin Kamleithner added 3 commits July 18, 2019 01:48

Add searchFieldLabel to SearchDelegate in order to show a custom hint…

69ec01c

… label.

Merge remote-tracking branch 'origin/master'

ecae87d

Add searchFieldLabel to SearchDelegate in order to show a custom hint…

4c1102b

… label.

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

googlebot added the cla: yes label Jul 18, 2019

dnfield requested a review from shihaohong July 22, 2019 21:36

shihaohong approved these changes Jul 24, 2019

View reviewed changes

Apply suggestions from code review

8420f39

Fix code samples in documentation

1acc04f

shihaohong reviewed Jul 24, 2019

View reviewed changes

shihaohong self-assigned this Jul 24, 2019

Martin Kamleithner added 3 commits July 24, 2019 17:48

Apply suggestions from code review

771e35a

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

0ebc373

Merge branch 'master' into addSearchFieldLabel

7ad1973

Add support for specifying textInputAction and keyboardType in Search…

311c0cd

…Delegate

shihaohong approved these changes Jul 24, 2019

View reviewed changes

Apply suggestions from code review

65cfdcc

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

Apply suggestions from code review

9bc7d4f

shihaohong added the waiting for tree to go green label Jul 29, 2019

shihaohong merged commit 9f08316 into flutter:master Jul 29, 2019

This was referenced Jul 29, 2019

Add option to override search hint for full screen material search #18831

Closed

Allow search page widget search field hint text and keyboard text input action to be customizable #28807

Closed

HansMuller removed the waiting for tree to go green label Jul 31, 2019

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

	/// The hint text to be shown when the search field is empty.
	/// The hint text that is shown in the search field when it is empty.

	testWidgets('Showing custom search hint', (WidgetTester tester) async {
	testWidgets('Custom searchFieldLabel value', (WidgetTester tester) async {

	testWidgets('Showing default search label if no custom label is given', (WidgetTester tester) async {
	testWidgets('Default searchFieldLabel is used when it is set to null', (WidgetTester tester) async {

Add searchFieldLabel to SearchDelegate in order to show a custom hint #36409

Add searchFieldLabel to SearchDelegate in order to show a custom hint #36409

Uh oh!

Conversation

knaeckeKami commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knaeckeKami commented Jul 24, 2019

Uh oh!

shihaohong commented Jul 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihaohong Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knaeckeKami commented Jul 24, 2019

Uh oh!

shihaohong commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knaeckeKami commented Jul 24, 2019

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shihaohong commented Jul 24, 2019

Uh oh!

samlythemanly commented Jul 24, 2019

Uh oh!

knaeckeKami commented Jul 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

knaeckeKami commented Jul 18, 2019 •

edited

Loading

shihaohong Jul 24, 2019 •

edited

Loading

shihaohong commented Jul 24, 2019 •

edited

Loading