Expandable Search - #17629

To reduce the risk of importing this demo along with all of the others, it's best if this is the only public class in the file.

Done.

NICE. You might want to add a comment that indicates that all of the gallery tests depend on this...

It might be better to put something in this drawer that has to do with the demo. Search limits maybe?

Let me think about this one...

It would be OK to put this expression on the previous line

AFAICT this delegate defines the search experience.

It would be helpful to mention query and showResultsPage here

It might be a bit more fluttery to define a ResultCard widget instead of this function

"overlay" might be a little confusing vis the Overlay widget. The docs tend to refer to the result of showSearchOverlay as a page, show maybe showSearchPage etc.

It would helpful to also mention [close] here

A slightly simpler way to create a vertical spacer is: const SizedBox(height: 64.0)

Developers may want to reuse some of this buildSuggestions method, since showing a list of strings with the query highlighted is common. You could simplify lifting the reusable code by creating a _SuggestionList widget, like:

return new _SuggestionList(
  query: query,
  suggestions: suggestions.map((int i) => '$i'), 
  onSelected: (String suggestion) { // A ValueChanged<String> callback
    query = suggestion;
    showResultsPage(context);    
  },
);

If you passed the search delegate along then an onSelected callback wouldn't be needed.

Love it!

extra blank line here

Maybe: Shows a full screen search experience in terms of a search page and a results page.

overlay => search page

"search experience" sounds pretty fancy; maybe just "search and results pages"

to the search => to the search page

in the underlying page while the search page fades in or out

Do you really want to provide a default value here? Are developers supposed to specify null to get delegate.query?

The reason I did it is that the SearchDelegate is potentially long-lived. If you do two searches back-to-back the expectation is that when the search page is brought up a second time the query field is empty again. That's what's enforced here. If developers want to diverge from the default behavior of resetting the query, they can do so here.

The docs don't appear to cover the return value.

changes and = changes.

Since I think we really intend for the suggestions to be based on the query string: The suggestions should be based on the [query] string ... And I think you should what the method is supposed to do if [query] is the empty string, since that's the default.

content => current value of

Typically an [IconButton] configured with a [BackButtonIcon] that exits the search with [close].

One can also use an [AnimatedIcon]...

while the => when the

return by => returned by

It's not really clear what this is for. In other words, if I'm building a SearchDelegate subclass what would I typically do with focusNode?

Right. I'll make it private.

Do SearchDelegate subclasses need to see this? If they really wanted to, they could remember if they'd most recently called showSuggestions or showResults

I'll make it private as well.

Same as earlier: do SearchDelegate subclassers need this?

Same.

users => user

animated => animates

It might be good to link this to buildResults more clearly. Like: Transition from the suggestions returned by [buildSuggestions] to the [query] results returned by [buildResults].

Suggest complicating this this a little (as with showResults) to make the link to buildSuggestions clear.

It would be helpful to include a point about suggestion widget onTap handlers typically calling [close] to return the search result. Just like buildSuggestions mentions what a typical suggestion onTap handler does.

extra blank line

It might be a little clearer if this was called queryTextController or something less generic.

trailing comma

{ goes on the previous line

We usually format this kind of thing like:

  const TestHomePage({
    this.foo,
    this.delegate,
    this.passInInitialQuery: false,
    this.initialQuery,
  });

I think it's possible for the original route to have been removed while the search dialog is up, which would probably trigger a strange failure mode. If developers start running into that, we could make this code more defensive. In a future PR.

Sounds good.