ABC-92 Add paging and server search for custom emojis to emoji picker #657

jwilander · 2018-01-24T16:37:04Z

Summary

Add paging and server search for custom emojis to emoji picker.

@hmhealey @stephenkiers let me know if I should approach this differently. This is my first time in the emoji picker code and it's a bit complex, so if you guys have better ideas on how to do this let me know :)

Ticket Link

https://mattermost.atlassian.net/browse/ABC-92

Checklist

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Has server changes (ABC-92 Add GET /emojis/name/{emoji_name} API endpoint mattermost#8142)
Has redux changes (ABC-92 Add actions for getting custom emoji by name and id mattermost-redux#373)
Has UI changes

jwilander · 2018-01-24T21:42:02Z

@jasonblais I loaded a bunch of emojis and it should be ready for testing

jasonblais

@jwilander As discussed briefly as an option yesterday, can we make a slight change where the next page of emojis load before you hit the end of the previous page?

Right now it looks like there are no more emoji at the bottom once you've scrolled down.

I'm 0/5 on how early we start loading without trying it out. Perhaps once you've reached the last 50 on the page (half of the emoji picker UI), we load the next 200?

jwilander · 2018-01-25T16:06:48Z

Yep, I'll make that change

mattermod · 2018-01-26T20:53:19Z

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

mattermod · 2018-01-26T20:55:22Z

Spinmint test server created at: http:https://i-06133eee40c5f4919.spinmint.com

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

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

Instance ID: i-06133eee40c5f4919

jwilander · 2018-01-26T21:03:17Z

@jasonblais this one is ready too

jasonblais

Performance-wise it feels better compared to current master. Scrolling through the picker feels smooth.

@jwilander Not related to this PR, but I get a blank screen initially when I switch to the custom emoji category on the picker (same as on master). Anything we can do there to improve it,
perhaps a loading screen?

@hmhealey @stephenkiers ready for your review.

stephenkiers · 2018-01-30T21:55:21Z

2 Questions:

I noticed that emojis aren't being loaded in the center window until they are loaded via the emoji picker. Is that simply because the other PR isn't in this one? Or is it a bug? See : https://www.youtube.com/watch?v=AMFTA_YKYko
are we saving the emojis in local storage once loaded so they aren't reloaded every time?

stephenkiers · 2018-01-30T21:57:30Z

I feel like this component group needs to be split into multiple, simpler components that do one thing each. However that is another task in and of itself.
I thought your approach looked good and seems to work good. :)

stephenkiers

I am approving in the assumption that the bug I put in the comment above is simply because the other related PR isn't loaded yet.

jwilander · 2018-01-30T22:02:07Z

Yes, that's just because the other PR hasn't been merged yet and I disabled the load all emoji on page load functionality for testing this PR (will revert that before merging)
No, currently we only store a few things in local storage right now. Agreed though, it is something we should look at in the future

jwilander · 2018-01-30T22:04:30Z

@jasonblais is this just before the custom emojis load in? If so yeah we can definitely add a loading indicator but I think there's still going to be some delay between that disappearing and the emojis loading in to cover the gap of time between when the getting metadata request completes and the get emoji images requests start going out. We could do a bunch of refactoring to make that gap not exist (at least from the user's perspective) but that's a lot more work

jasonblais · 2018-01-30T22:13:22Z

@jwilander I see that with the system emoji too. If I click on a category, the emoji picker is just blank, after which the emoji appear.

Should I just create a ticket to look into it?

jwilander · 2018-01-30T22:15:08Z

Oh interesting.. I guess that's just waiting for the images to load in. Yeah I'd say that's another ticket

jasonblais · 2018-01-30T22:18:31Z

Cool, ticket: https://mattermost.atlassian.net/browse/ICU-684

stephenkiers · 2018-01-31T04:04:15Z

That should be easy to fix. I will take that one next sprint.

hmhealey

Looks good unless we have a batched API for getting emojis that I don't know about

hmhealey · 2018-01-31T14:19:15Z

actions/emoji_actions.jsx

+ const missingEmojis = recentEmojis.filter((name) => !emojiMap.has(name));
+
+ missingEmojis.forEach((name) => {
+ EmojiActions.getCustomEmojiByName(name)(dispatch, getState);


It would be nice if we had a bulk API for this. As it is, we're making up to 27 requests for this

I'm not sure I agree, with this there will be caching by both the proxy and the browser. With a bulk request it's going to always go the whole way to the server

The ABC team does have a ticket to look into which is really better and when but using the force I think it's going to be better to have more individual GETs most of the time
https://mattermost.atlassian.net/browse/ABC-163

hmhealey · 2018-01-31T14:26:44Z

components/emoji_picker/emoji_picker_overlay.jsx

@@ -5,7 +5,7 @@ import PropTypes from 'prop-types';
 import React from 'react';
 import {Overlay} from 'react-bootstrap';

-import EmojiPicker from './emoji_picker.jsx';
+import EmojiPicker from './';


This import state looks really weird to me, but it makes sense without moving a bunch of files around

Yeah it weirded me out a bit but I didn't want to have to go rearrange a bunch of files

hmhealey · 2018-01-31T14:30:30Z

package.json

@@ -27,7 +27,7 @@
 "localforage": "1.5.6",
 "localforage-observable": "1.4.0",
 "marked": "mattermost/marked#802e981ade71149a497cbe79d12b8a3f82f7657e",
- "mattermost-redux": "mattermost/mattermost-redux",
+ "mattermost-redux": "mattermost/mattermost-redux#abc-92-no-emoji",


Is this supposed to be changed here?

It was while we were testing, I'll update it back to normal

jwilander added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter labels Jan 24, 2018

jwilander added this to the v4.7.0 milestone Jan 24, 2018

jwilander requested review from stephenkiers, hmhealey and jasonblais January 24, 2018 16:37

jwilander added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Jan 24, 2018

mattermost deleted a comment from mattermod Jan 24, 2018

jwilander force-pushed the abc-92 branch from 722ef43 to 76cf045 Compare January 24, 2018 20:54

jwilander added the Setup Old Test Server Triggers the creation of a test server label Jan 24, 2018

jasonblais suggested changes Jan 25, 2018

View reviewed changes

jasonblais self-assigned this Jan 25, 2018

jwilander force-pushed the abc-92 branch from 76cf045 to eacf9dd Compare January 26, 2018 20:37

jwilander removed the Setup Old Test Server Triggers the creation of a test server label Jan 26, 2018

mattermost deleted a comment from mattermod Jan 26, 2018

jwilander added the Setup Old Test Server Triggers the creation of a test server label Jan 26, 2018

jasonblais approved these changes Jan 30, 2018

View reviewed changes

jasonblais removed the 1: PM Review Requires review by a product manager label Jan 30, 2018

jasonblais removed their assignment Jan 30, 2018

stephenkiers approved these changes Jan 30, 2018

View reviewed changes

hmhealey reviewed Jan 31, 2018

View reviewed changes

jwilander force-pushed the abc-92 branch from eacf9dd to 97539fb Compare January 31, 2018 15:14

jwilander requested a review from hmhealey February 2, 2018 15:54

hmhealey approved these changes Feb 2, 2018

View reviewed changes

hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Feb 2, 2018

jwilander added 4 commits February 2, 2018 11:04

Add paging and server search for custom emojis to emoji picker

1ba8430

Don't try to load custom emojis when disabled

d9ca560

Updates per feedback

1c1ff98

Update mattermost-redux

b840354

jwilander force-pushed the abc-92 branch from 97539fb to b840354 Compare February 2, 2018 16:06

jwilander removed the Setup Old Test Server Triggers the creation of a test server label Feb 2, 2018

jwilander merged commit 6f9fa7b into master Feb 2, 2018

jwilander deleted the abc-92 branch February 2, 2018 17:13

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Feb 2, 2018

lindalumitchell added the Tests/Done Release tests have been written label Feb 15, 2018

hmhealey pushed a commit that referenced this pull request Aug 28, 2020

Fixing other search types. (#657)

52be987

hmhealey pushed a commit that referenced this pull request Mar 17, 2021

Fixing other search types. (#657)

070adc8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ABC-92 Add paging and server search for custom emojis to emoji picker #657

ABC-92 Add paging and server search for custom emojis to emoji picker #657

jwilander commented Jan 24, 2018

jwilander commented Jan 24, 2018

jasonblais left a comment

jwilander commented Jan 25, 2018

mattermod commented Jan 26, 2018

mattermod commented Jan 26, 2018

jwilander commented Jan 26, 2018

jasonblais left a comment •

edited

Loading

stephenkiers commented Jan 30, 2018

stephenkiers commented Jan 30, 2018

stephenkiers left a comment

jwilander commented Jan 30, 2018 •

edited

Loading

jwilander commented Jan 30, 2018

jasonblais commented Jan 30, 2018 •

edited

Loading

jwilander commented Jan 30, 2018

jasonblais commented Jan 30, 2018

stephenkiers commented Jan 31, 2018

hmhealey left a comment

hmhealey Jan 31, 2018

jwilander Jan 31, 2018

jwilander Jan 31, 2018 •

edited

Loading

hmhealey Jan 31, 2018

jwilander Jan 31, 2018

hmhealey Jan 31, 2018

jwilander Jan 31, 2018

ABC-92 Add paging and server search for custom emojis to emoji picker #657

ABC-92 Add paging and server search for custom emojis to emoji picker #657

Conversation

jwilander commented Jan 24, 2018

Summary

Ticket Link

Checklist

jwilander commented Jan 24, 2018

jasonblais left a comment

Choose a reason for hiding this comment

jwilander commented Jan 25, 2018

mattermod commented Jan 26, 2018

mattermod commented Jan 26, 2018

jwilander commented Jan 26, 2018

jasonblais left a comment • edited Loading

Choose a reason for hiding this comment

stephenkiers commented Jan 30, 2018

stephenkiers commented Jan 30, 2018

stephenkiers left a comment

Choose a reason for hiding this comment

jwilander commented Jan 30, 2018 • edited Loading

jwilander commented Jan 30, 2018

jasonblais commented Jan 30, 2018 • edited Loading

jwilander commented Jan 30, 2018

jasonblais commented Jan 30, 2018

stephenkiers commented Jan 31, 2018

hmhealey left a comment

Choose a reason for hiding this comment

hmhealey Jan 31, 2018

Choose a reason for hiding this comment

jwilander Jan 31, 2018

Choose a reason for hiding this comment

jwilander Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

hmhealey Jan 31, 2018

Choose a reason for hiding this comment

jwilander Jan 31, 2018

Choose a reason for hiding this comment

hmhealey Jan 31, 2018

Choose a reason for hiding this comment

jwilander Jan 31, 2018

Choose a reason for hiding this comment

jasonblais left a comment •

edited

Loading

jwilander commented Jan 30, 2018 •

edited

Loading

jasonblais commented Jan 30, 2018 •

edited

Loading

jwilander Jan 31, 2018 •

edited

Loading