autocomplete: Add user avatars to user-mention autocompletes #590
Conversation
sm-sayedi
force-pushed
the
issue-227-avatars-in-user-mention-autocompletes
branch
2 times, most recently
from
a78d4e0 to
0cf6caf
Compare
|
Thanks for the review. Revision pushed with tests included. |
sm-sayedi
force-pushed
the
issue-227-avatars-in-user-mention-autocompletes
branch
from
0cf6caf to
5633215
Compare
There was a problem hiding this comment.
Thanks @sm-sayedi! Generally this looks good. Various comments below, all fairly small.
| @@ -39,6 +41,8 @@ Future<Finder> setupToComposeInput(WidgetTester tester, { | |||
| messages: [message], | |||
| ).toJson()); | |||
|
|
|||
| prepareBoringImageHttpClient(); | |||
There was a problem hiding this comment.
With this call, it becomes part of the interface of this setupToComposeInput function that its caller needs to set debugNetworkImageHttpClientProvider back to null before the end of the test. So this function should gain a note to that effect in its dartdoc, just like prepareBoringImageHttpClient has.
(It's regrettable that that reset can't be done using addTearDown, so that callers and callers' callers wouldn't have to care like this. There's an open issue upstream about that, relating to debugNetworkImageHttpClientProvider and a bunch of other Flutter test knobs of a similar flavor. I don't have the link offhand, but it'd be good to track it down again and then put a comment on prepareBoringImageHttpClient pointing to that.)
There was a problem hiding this comment.
I found a closed issue upstream. This one also seemed closer to this, but wasn't sure to add it to the prepareBoringImageHttpClient comment.
There was a problem hiding this comment.
Thanks for looking! Found the one I was thinking of (which it turns out is one I filed 🙂), and added a comment: 68116a2
Looks like there was a backlink to it in the second issue you found:
as that issue is basically a different specific case of the issue I had in mind.
The WildcardMentionAutocompleteResult and UserGroupMentionAutocompleteResult classes are not used, so better to remove them for now.
sm-sayedi
force-pushed
the
issue-227-avatars-in-user-mention-autocompletes
branch
from
5633215 to
957338d
Compare
|
Thanks for the review. Revision pushed. Also requested to work on issue 100 simultaneously! |
test/widgets/autocomplete_test.dart
Outdated
| Image(image: NetworkImage(url: var imageUrl)) when imageUrl == url => true, | ||
| _ => false, |
There was a problem hiding this comment.
nit: because the first pattern is so long, the result "true" gets a bit hard to spot when it's off at the end of that line. So:
| Image(image: NetworkImage(url: var imageUrl)) when imageUrl == url => true, | |
| _ => false, | |
| Image(image: NetworkImage(url: var imageUrl)) when imageUrl == url | |
| => true, | |
| _ => false, |
| check(tester.widgetList(find.text('User One'))).isEmpty(); | ||
| tester.widget(find.text('User Two')); | ||
| tester.widget(find.text('User Three')); | ||
| checkUserShown(user1, store, expected: false); |
There was a problem hiding this comment.
This version is fine. But for other situations where the shared context is more verbose than just store, and so it's more of a pain to have to pass it each time, there are two solutions we often use:
- The helper function can be local within the test function, so that it sees the same
storeas a local. - The context like
storecan be alatevariable that's declared up at the top of a test group, or of the whole testmain, and that gets initialized afresh by a helper function that each test case calls. Those helpers are often namedprepareor of the formprepareFoo.
| /// | ||
| /// The caller must set [debugNetworkImageHttpClientProvider] back to null | ||
| /// before the end of the test. | ||
| FakeImageHttpClient prepareBoringImageHttpClient() { |
There was a problem hiding this comment.
nit: prefix for this commit:
f3aebc9 autocomplete [nfc]: Pull prepareBoringImageHttpClient out to toplevel
should be autocomplete test [nfc]:, to indicate that it's only touching the test code
(And then the "nfc" part means that on top of that, it has only an NFC effect on even the test code.)
There was a problem hiding this comment.
Oh and looking again, it's not really about the autocomplete tests in particular, so autocomplete isn't quite right.
I'll make it content test [nfc], since it is in that file. Could also have made it just test [nfc], since the point is it's now exporting this for other tests.
Moving `prepareBoringImageHttpClient` method to the toplevel of the file will make it reusable in other files.
gnprice
force-pushed
the
issue-227-avatars-in-user-mention-autocompletes
branch
from
957338d to
919cf12
Compare
This follows up on this discussion: #590 (comment)
|
Thanks @gnprice for all this and for the instructive comments!! |
When mentioning users, their avatars are shown beside their names in the autocomplete popup.
Fixes: #227