Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

ABC-91 Add server request to emoji autocomplete #648

Merged
merged 2 commits into from
Feb 5, 2018
Merged

ABC-91 Add server request to emoji autocomplete #648

merged 2 commits into from
Feb 5, 2018

Conversation

jwilander
Copy link
Member

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

@jwilander jwilander added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter labels Jan 22, 2018
@jwilander jwilander added this to the v4.7.0 milestone Jan 22, 2018
// 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) => {
Copy link
Contributor

@stephenkiers stephenkiers Jan 23, 2018

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

@jasonblais jasonblais added the Setup Old Test Server Triggers the creation of a test server label Jan 23, 2018
@jasonblais jasonblais self-assigned this Jan 23, 2018
@jasonblais
Copy link
Contributor

@jwilander Cannot type more than one character following a comma, and hence can't test the autocomplete.
image

Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@jwilander
Copy link
Member Author

jwilander commented Jan 23, 2018

Oh sorry, this isn't testable yet. It needs some redux and server PRs to go in first

@jwilander jwilander added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed Setup Old Test Server Triggers the creation of a test server labels Jan 23, 2018
@mattermost mattermost deleted a comment from mattermod Jan 23, 2018
@mattermost mattermost deleted a comment from mattermod Jan 23, 2018
@mattermost mattermost deleted a comment from mattermod Jan 23, 2018
@jwilander jwilander added Setup Old Test Server Triggers the creation of a test server and removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Setup Old Test Server Triggers the creation of a test server labels Jan 23, 2018
@mattermost mattermost deleted a comment from mattermod Jan 23, 2018
@mattermost mattermost deleted a comment from mattermod Jan 23, 2018
@mattermost mattermost deleted a comment from mattermod Jan 23, 2018
@jwilander jwilander closed this Jan 23, 2018
@jwilander jwilander reopened this Jan 23, 2018
@jwilander jwilander added the Setup Old Test Server Triggers the creation of a test server label Jan 23, 2018
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@mattermod
Copy link
Contributor

Spinmint test server created at: https://i-0b61ded4fe926bc7c.spinmint.com

Test Account 1: Email: [email protected] | Password: passwd

Test Account 2: Email: [email protected] | Password: passwd

Instance ID: i-0b61ded4fe926bc7c

@jwilander
Copy link
Member Author

@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

@jwilander
Copy link
Member Author

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.

@jasonblais
Copy link
Contributor

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

@jasonblais
Copy link
Contributor

I'll post about this in UX channel to get other people's opinions.

@jasonblais
Copy link
Contributor

Should hear back soon, I think we either go with the current implementation, or always load local results first before server-side results.

@jasonblais
Copy link
Contributor

@jwilander UX team leaning towards the implementation we have now. Question though:

Is there no way of caching the data beyond a single session? So that it doesn't have to pull down a fresh copy every time i refresh if its not necessary? It would be good to reduce how frequently someone would end up encountering the issue.

@jwilander
Copy link
Member Author

jwilander commented Feb 1, 2018

Yes, it's possible to do that. The RN apps do that. It'll be more work though (4-8 mana)

@jasonblais jasonblais removed the 1: PM Review Requires review by a product manager label Feb 2, 2018
@jasonblais jasonblais removed their assignment Feb 2, 2018
@jwilander jwilander removed the Setup Old Test Server Triggers the creation of a test server label Feb 2, 2018
@hmhealey hmhealey removed the 2: Dev Review Requires review by a core commiter label Feb 5, 2018
@hmhealey hmhealey merged commit 5c3aa17 into master Feb 5, 2018
@hmhealey hmhealey deleted the abc-91 branch February 5, 2018 15:20
@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label Feb 6, 2018
@amyblais amyblais added Docs/Not Needed Does not require documentation Changelog/Done Required changelog entry has been written labels Feb 7, 2018
hmhealey pushed a commit that referenced this pull request Aug 28, 2020
hmhealey pushed a commit that referenced this pull request Mar 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Not Needed Does not require new release tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants