-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Autocomplete (Material) #73753
Autocomplete (Material) #73753
Conversation
c9cfad5
to
47f3205
Compare
/// See also: | ||
/// * [RawAutocomplete], which is what Autocomplete is built upon, and which | ||
/// contains more detailed examples. |
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 think the "See also" section usually come after everything else, so this should go after the Dartpad examples below this
_AutocompleteState<T> createState() => _AutocompleteState<T>(); | ||
} | ||
|
||
class _AutocompleteState<T extends Object> extends State<Autocomplete<T>> { |
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.
Does this have to be stateful?
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.
Ah thank you, how did I even miss that...
/// See also: | ||
/// * [Autocomplete], which is a Material-styled implementation that is based | ||
/// on RawAutocomplete. |
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.
Same question here. Isn't "See also" usually at the very end of an API doc? I might be wrong though
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.
You seem to be right to me as I look around the codebase. I'll change it.
/// this.email, | ||
/// this.name, |
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.
NNBD
/// this.email, | |
/// this.name, | |
/// required this.email, | |
/// required this.name, |
child: Material( | ||
elevation: 4.0, | ||
child: Container( | ||
height: 200.0, | ||
child: ListView.builder( | ||
padding: const EdgeInsets.all(8.0), |
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 these specs come from somewhere?
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.
No, there is no specific Material autocomplete spec. I've just picked some numbers that seem to look ok to me... But I'm happy to change them if anyone with a better design sense wants me to.
return GestureDetector( | ||
onTap: () { | ||
onSelected(option); | ||
}, | ||
child: ListTile( | ||
title: Text(displayStringForOption(option)), | ||
), | ||
); |
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.
What about:
return GestureDetector( | |
onTap: () { | |
onSelected(option); | |
}, | |
child: ListTile( | |
title: Text(displayStringForOption(option)), | |
), | |
); | |
return ListTile( | |
onTap: () { | |
onSelected(option); | |
}, | |
title: Text(displayStringForOption(option)), | |
); |
This gives it an ink splash so there is some responsiveness for users to confirm that their action has some effect. This would be similar to Angular's autocomplete: https://material.angular.io/components/autocomplete/overview. However, I don't know if this accounts for the highlight on hover effect on desktop/web, which would be pretty important.
Edit: I wonder if ListTile is the correct solution for this, since the dropdown selection font size is by default larger than the text field font size, which looks a bit weird. Could we just use an InkWell
or InkResponse
widget here, and have onHover
defined. Then, we could use the menu theming for defining the padding for each menu item based on platform: https://material.io/components/menus#specs
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.
elevation: 4.0, | ||
child: Container( | ||
height: 200.0, | ||
child: ListView.builder( |
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.
ListView behaves funky when the TextField itself is constrained:
Sample app
import 'package:flutter/material.dart';
void main() {
runApp(MyApp());
}
class MyApp extends StatelessWidget {
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
theme: ThemeData(
primarySwatch: Colors.blue,
),
home: MyHomePage(title: 'Flutter Demo Home Page'),
);
}
}
class MyHomePage extends StatefulWidget {
MyHomePage({Key key, this.title}) : super(key: key);
final String title;
@override
_MyHomePageState createState() => _MyHomePageState();
}
class _MyHomePageState extends State<MyHomePage> {
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
title: Text(widget.title),
),
body: Center(
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: <Widget>[
SizedBox(
width: 200,
child: AutocompleteBasicExample(),
),
],
),
),
);
}
}
class AutocompleteBasicExample extends StatelessWidget {
AutocompleteBasicExample({Key key}) : super(key: key);
final List<String> _kOptions = <String>[
'aardvark',
'bobcat',
'chameleon',
'dingo',
'elephant',
'flamingo',
'goose',
'hippopotamus',
'iguana',
'jaguar',
'koala',
'lemur',
'mouse',
'northern white rhinocerous',
];
@override
Widget build(BuildContext context) {
return Autocomplete<String>(
optionsBuilder: (TextEditingValue textEditingValue) {
if (textEditingValue.text == '') {
return const Iterable<String>.empty();
}
return _kOptions.where((String option) {
return option.contains(textEditingValue.text.toLowerCase());
});
},
optionsViewBuilder: (
BuildContext context,
AutocompleteOnSelected<String> onSelected,
Iterable<String> options,
) {
return Align(
alignment: Alignment.topLeft,
child: Material(
elevation: 4.0,
child: Container(
height: 200.0,
child: ListView.builder(
padding: const EdgeInsets.all(8.0),
itemCount: options.length,
itemBuilder: (BuildContext context, int index) {
final String option = options.elementAt(index);
return InkWell(
onHover: (bool enableFeedback) {},
onTap: () {
onSelected(option);
},
child: Container(
constraints: const BoxConstraints(minHeight: 24.0),
alignment: AlignmentDirectional.centerStart,
child: Text(option),
),
);
},
),
),
),
);
},
onSelected: (String selection) {
print('You just selected $selection');
},
);
}
}
Basically, the ListView itself extends beyond the text field's length when it should end. I wonder if there's an easy way to figure the width of the text field out and limit it accordingly?
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 still haven't taken a look at this but I'll try to come back to it later today. I thought I made an example of this at some point, but maybe I broke it again with all the changes or something.
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.
The example I was thinking of (here), just manually sets the width of the optionsViewBuilder to the same as the field. Hans was pushing for the ability to handle this case, and this is what I came up with for the short term.
But I really feel like I might be able to get the width of the field using the FocusNode and make this work nicely right now... I'll take another closer look tomorrow and see, otherwise I'll postpone it for a later PR.
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 played around with a bunch of different options for this, and my conclusion is that we should come back to this in another PR if you're ok with that. I've created an issue to discuss our options: #74509
The good news is that it should be possible to do anything we want here. I was able to get the size of the field via the FocusNode (even when the field is not created with fieldViewBuilder). I was also able to get the constraints of the Autocomplete widget at its point in the tree using a LayoutBuilder.
The problem is I'm not fully satisfied with any one solution here. I could set the width to the width of the field, but some users may want it to be full-width or the width of the Autocomplete instead (e.g. split field and options). This is quickly infringing on the idea of flat options, which I mentioned in the design doc and we should also support at some point.
So, I think we should leave the behavior the same as RawAutocomplete for now and come back to this with a full solution. I've proposed another builder method that would allow full customizability (see the issue linked above).
Let me know what you think. For now, if users want to achieve a narrow field with results that match, they would need to manually set the width of each to be the same, as it is with RawAutocomplete (that's how it's done in the example I was thinking of).
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.
That makes sense! Happy to move forward with what we have for now and come back to this in a later PR.
@shihaohong I'm going to go ahead and put the "waiting for tree to go green" label on this even though I don't have a final LGTM from you since it looks like you were on board with everything. If not, feel free to remove the label, or if it's already merged, I'm happy to revert. |
This pull request is not suitable for automatic merging in its current state.
|
Looks like the bot won't let me do that. @shihaohong if you review this on your Monday, can you just merge it or put the label on 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.
LGTM! Wasn't sure if you were done making edits since when I was looking at it since I saw some recent commits.
Awesome, thanks for the Saturday review! |
This widget contains a minor bug. 2021-09-16-20-51-51.mp4 |
@ArinFaraj Thanks for catching that, could you open a new issue and tag me and @darrenaustin? |
This is the high-level Autocomplete Material widget. Previous work on RawAutocomplete did all of the heavy lifting, and this class just creates a Material-styled wrapper that is easy to use.
Tests
Breaking change
No.