-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Create SearchBar and SearchBarTheme
#122309
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
Conversation
0b8f764 to
c021b4c
Compare
SearchBar and SearchTheme
SearchBar and SearchThemeSearchBar and SearchBarTheme
da283f3 to
4403dac
Compare
guidezpl
left a comment
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.
Nice work, just gave it a quick first pass.
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.
Should this file be name search_bar.dart?
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.
Because I think the search bar will only be used for the widget SearchAnchor which will be created in the future PR, so just want to put them in the same file.
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.
I don't know what search anchor refers to, I couldn't find a mention in the guidelines
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.
Sorry for the confusion. To open a search view, I'm creating a SearchAnchor widget so that we can get all the anchor(SearchBar or IconButton or any widget) information, such as size, position and etc. Here's the design doc https://docs.google.com/document/d/1_WOcgddfJ7OyYyaCJhkIWgOYywj2jDeMooFsKg012B4/edit#
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.
Thanks! I'm a bit confused because it talks about the introduction of 2 widgets, and the SearchView section only mentions SearchAnchor. Are these terms used interchangeably and should s/anchor/view/?
If SearchView and SearchAnchor are distinct widgets, is there a way to avoid having 3 public widgets? 2 would be ideal.
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.
I see. I think we will only need SearchBar and SearchAnchor widgets. Will update the design soon! Thanks:)
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.
Should this be in the constructor docs?
70e4a5e to
7557923
Compare
HansMuller
left a comment
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.
Mostly just small stuff. This is looking good.
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.
Do we really want to prevent developers from using 3 or more trailing widgets? Although it's definitely not advisable, it doesn't seem worth asserting if a developer founds a justification for it.
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.
I see. Removed this assertion.
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.
this can be on one line
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.
Fixed this and others! I think the IDE did this kind of format automatically. Sorry for not catching this.
08d118c to
ea22491
Compare
HansMuller
left a comment
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.
LGTM
| /// The surface tint color of the button's [Material]. | ||
| /// | ||
| /// See [Material.surfaceTintColor] for more details. | ||
| /// | ||
| /// If null, the value of [SearchBarThemeData.surfaceTintColor] will be used. | ||
| /// If this is also null, then the default value is [ColorScheme.surfaceTint]. | ||
| final MaterialStateProperty<Color?>? surfaceTintColor; | ||
|
|
||
| /// The highlight color that's typically used to indicate that | ||
| /// the button is focused, hovered, or pressed. |
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.
Can you clarify what "the button" is?
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.
Should be "Search bar". Thanks for catching this. Fixed!
guidezpl
left a comment
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.
LGTM pending documentation update for surfaceTintColor and overlayColor
This PR is to create a
SearchBarwidget which is the default trigger for aSearchView.A
SearchAnchorwidget will be created soon to show search view.In order to use the
SearchBarwith the new Material 3 colors, turn on theuseMaterial3flag in theThemeData:Related to #117483.
Pre-launch Checklist
///).