Allow search page widget search field hint text and keyboard text input action to be customizable by samlythemanly · Pull Request #28807 · flutter/flutter

samlythemanly · 2019-03-03T20:30:56Z

Description

Currently, the search page widget's hint text and keyboard text input action are hardcoded to the material localization of 'Search' and the search text input action, respectively. This prevents further customization where search functionality is desired, but the nuance of the action isn't exactly "searching" (e.g. adding a location via Google Maps Places API autocomplete).

Added a getHintText function (that takes in a BuildContext) and a textInputAction field to SearchDelegate. These default to the previous hardcoded attributes that were present in SearchPage.

These changes are purely aesthetic changes; no additional functionality is provided.
All tests still pass.

Related Issues

N/A

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.
My PR includes tests for all changed/updated/fixed behaviors (See Test Coverage).
All existing and new tests are passing.
I updated/added relevant documentation (doc comments with ///).
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
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).
No, this is not a breaking change.

justinmc

@somarkoe Thanks for submitting this and sorry for the lack of reviewers. Looks like a simple improvement to make Search a lot more useful, I'm all on board.

Just a discussion below to decide the best way to do this. There was a Linux test failing, but I think it was flaky so I reran it. We'll see.

mikuhl-dev · 2019-05-08T19:47:39Z

While you're at it, please do keyboardType: #29983
You don't always search with text.

justinmc · 2019-05-09T17:10:57Z

I agree with the idea to add keyboardType to this as well.

Looks like all tests are passing now. @somarkoe are you still available to get this PR in?

samlythemanly · 2019-05-10T18:07:45Z

Yes! Sorry, been swamped by work and haven't touched the project that deals with this in a while. I'll add the keyboard type request to it as well.

justinmc · 2019-05-10T19:28:26Z

Awesome, thank you @somarkoe. Update the PR when you get a chance and I'll be happy to help you get this in.

mikuhl-dev · 2019-05-10T19:37:16Z

@justinmc Perhaps will you be able to get #28165 in too? There's so much about the search page that is hard coded and it shouldn't be.

justinmc · 2019-05-10T19:48:54Z

Yeah that one looks straightforward to me. The original reviewer @HansMuller isn't around today but I'll talk it through with him when he's back.

mikuhl-dev · 2019-06-08T10:48:45Z

Abandoned? :(

samlythemanly · 2019-06-08T20:04:02Z

Oops, been a while since I've used git.

Reopened and made the relevant changes. Sorry it took so long!

mikuhl-dev · 2019-06-08T22:48:05Z

😍 merge this ish!

mikuhl-dev · 2019-06-08T22:55:26Z

Only thing that could be added on is #30388 / #28165 (seems like they are the same 1 line of code)

justinmc · 2019-06-10T18:25:06Z

@somarkoe Thanks for following up and no worries about the delay!

Sorry to drag this out longer, but I realized that this should update the tests as well. This test should probably say "keyboard shows search button by default" now.

Then, could you add a separate test that tests a different textInputAction? Just duplicate that other test and add textInputAction to the TestHomePage parameters.

After that this is good to go and we can merge.

justinmc · 2019-06-10T18:25:57Z

@MichaelPriebe I'll try to see if we can get things moving on those 2 other PRs you referenced.

mikuhl-dev · 2019-06-15T12:12:05Z

@justinmc how's this going?

justinmc · 2019-06-17T15:29:10Z

Waiting on @somarkoe for the changes mentioned above. Should be quick changes though.

mikuhl-dev · 2019-06-22T00:43:25Z

@somarkoe come thru

mikuhl-dev · 2019-06-23T00:55:45Z

@justinmc What do you think?

…action, and keyboard text input type to be customizable. [Description] Currently, the search page widget's hint text and keyboard text input action are hardcoded to the material localization of 'Search' and the search text input action, respectively, and the text input type defaults to that defined in `TextField`. This prevents further customization where search functionality is desired, but the nuance of the action isn't exactly "searching" (e.g. adding a location via Google Maps Places API autocomplete). [Changes] Added `hintText`, `textInputAction`, and `keyboardType` fields to `SearchDelegate`. These default to the previous hardcoded attributes that were present in `SearchPage` (or `null`). [Testing] Added a new unit test to cover the new `textInputAction` case (and modified the existing one's name to indicate it's the default behavior). All existing tests still pass, as well.

samlythemanly · 2019-06-23T01:08:42Z

I made the fields non-final since apparently overriding fields when a class is extended is no bueno, and this abstract class is one meant to be extended and not implemented (since it has functions with default behaviors).

mikuhl-dev · 2019-06-23T16:45:20Z

Excited for this, can finally ditch my very hacky extension of the search delegate.

mikuhl-dev · 2019-06-24T17:07:35Z

@justinmc Is it ready now?

mikuhl-dev · 2019-06-25T15:05:46Z

Lets get this pull a'mergin'!

justinmc

Sorry for the delay! I talked to @HansMuller to get some tips for the non-final problem.

justinmc · 2019-06-25T17:11:32Z

 /// for another [showSearch] call.
 abstract class SearchDelegate<T> {

+  /// The type of action button to use for the keyboard.


We could keep the fields final by using a constructor instead like this:

const SearchDelegate({ this.keyboardType, this.hintText, this.textInputAction = TextInputAction.search, });

Then below, make all of the fields final and remove the default assignment from textInputAction.

justinmc · 2019-06-25T17:14:20Z

+    if (textInputAction != null) {
+      this.textInputAction = textInputAction;
+    }
+  }


Then with the change in my other comment, you can user super here:

... }) : super( textInputAction: textInputAction, );

mikuhl-dev · 2019-06-28T15:58:13Z

@somarkoe What do you think about the suggestions?

mikuhl-dev · 2019-07-07T01:03:06Z

@somarkoe Would love to hear what you think about the suggested changes.

samlythemanly · 2019-07-07T01:30:06Z

So sorry! Was on vacation and now I'm moving 😅

Ill take a look ASAP!

shihaohong · 2019-07-29T22:42:42Z

I'm going to close this since #36409 has been merged. Thank you @somarkoe for your initial work with this!

googlebot added the cla: yes label Mar 3, 2019

goderbauer added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Mar 4, 2019

justinmc self-requested a review May 1, 2019 17:20

justinmc reviewed May 1, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/material/search.dart Outdated

samlythemanly closed this Jun 8, 2019

samlythemanly force-pushed the master branch from 7ccd252 to 1a56015 Compare June 8, 2019 19:59

samlythemanly reopened this Jun 8, 2019

mikuhl-dev mentioned this pull request Jun 11, 2019

Add hintStyle in SearchDelegate #30388

Merged

7 of 9 tasks

samlythemanly force-pushed the master branch 2 times, most recently from e52b10a to 36bb89a Compare June 22, 2019 22:12

samlythemanly force-pushed the master branch from 36bb89a to 819a194 Compare June 23, 2019 01:06

justinmc reviewed Jun 25, 2019

View reviewed changes

shihaohong mentioned this pull request Jul 24, 2019

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

Merged

8 of 9 tasks

shihaohong closed this Jul 29, 2019

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

Conversation

samlythemanly commented Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikuhl-dev commented May 8, 2019

Uh oh!

justinmc commented May 9, 2019

Uh oh!

samlythemanly commented May 10, 2019

Uh oh!

justinmc commented May 10, 2019

Uh oh!

mikuhl-dev commented May 10, 2019

Uh oh!

justinmc commented May 10, 2019

Uh oh!

mikuhl-dev commented Jun 8, 2019

Uh oh!

samlythemanly commented Jun 8, 2019

Uh oh!

mikuhl-dev commented Jun 8, 2019

Uh oh!

mikuhl-dev commented Jun 8, 2019

Uh oh!

justinmc commented Jun 10, 2019

Uh oh!

justinmc commented Jun 10, 2019

Uh oh!

mikuhl-dev commented Jun 15, 2019

Uh oh!

justinmc commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikuhl-dev commented Jun 22, 2019

Uh oh!

mikuhl-dev commented Jun 23, 2019

Uh oh!

samlythemanly commented Jun 23, 2019

Uh oh!

mikuhl-dev commented Jun 23, 2019

Uh oh!

mikuhl-dev commented Jun 24, 2019

Uh oh!

mikuhl-dev commented Jun 25, 2019

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

justinmc Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

mikuhl-dev commented Jun 28, 2019

Uh oh!

mikuhl-dev commented Jul 7, 2019

Uh oh!

samlythemanly commented Jul 7, 2019

Uh oh!

shihaohong commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

samlythemanly commented Mar 3, 2019 •

edited

Loading

justinmc commented Jun 17, 2019 •

edited

Loading