-
Notifications
You must be signed in to change notification settings - Fork 2.7k
ABC-91 Add server request to emoji autocomplete #648
Conversation
// force the selection to be cleared since the order of elements may have changed | ||
SuggestionStore.clearSelection(suggestionId); | ||
// Sort the emoticons so that emoticons starting with the entered text come first | ||
matched.sort((a, b) => { |
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 feel like this method should be moved out of here, for easier reading.
matched = sortWithPreferenceGivenToPrefixMatches(matched, partialName);
1/5
@jwilander Cannot type more than one character following a comma, and hence can't test the autocomplete. |
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.
See above
Oh sorry, this isn't testable yet. It needs some redux and server PRs to go in first |
|
Spinmint test server created at: https://i-0b61ded4fe926bc7c.spinmint.com Test Account 1: Email: Test Account 2: Email: Instance ID: i-0b61ded4fe926bc7c |
@jasonblais This one should be ready for testing and has a bunch of custom emoji. For some reason the spinmints weren't getting the correct redux commits so I had to manually mess with them to get it to work |
I should also note that I disabled loading all emoji on page load so that this can be properly tested, That does mean that posts might show the text instead of the custom emoji, but that's expected and won't happen when it's merged. |
If I throttle my connection to a lower network speed, there's definitely a degradation in the user experience. The worry I have is that it's worse here, given the emoji I was looking at may get hidden after all the custom emoji are loaded (whereas for channels, the client-side results are always listed first, and the focus isn't lost). Let's discuss during our 1-1 |
I'll post about this in UX channel to get other people's opinions. |
Should hear back soon, I think we either go with the current implementation, or always load local results first before server-side results. |
@jwilander UX team leaning towards the implementation we have now. Question though:
|
Yes, it's possible to do that. The RN apps do that. It'll be more work though (4-8 mana) |
Summary
Add server request to emoji autocomplete. The diff looks like I did a lot but really I just encapsulated some functionality into a function and added the server request.
@jasonblais this is going to suffer from the same "server results popping in" issue that the other autocomplete functions have
Ticket Link
https://mattermost.atlassian.net/browse/ABC-91