Emoji autocomplete 🎉 ⚡ #1038

maximbaz · 2017-04-07T22:16:28Z

As agreed in #541, I've migrated the code from Surfingkeys to add Emoji autocomplete. I also stripped out some code which is unnecessary for Wire.

I've made it to function properly, and although I'm planning to test more, I think now it's a good time to ask you guys to start giving some feedback on it. Give it a try 😉

General notes:

I placed the list of all emojis in the /app/image folder, but strictly speaking it is not an image. Any suggestions on where to better place it? My only need is to be able to download this file from javascript, so it must be accessible as a static resource.
I placed the whole code in a separate file, because I wrote it in javascript due to Drop CoffeeScript #891 and due to Surfingkeys being written in javascript. The new ViewModel looks a bit artificial, let me know if you have some better ideas how to organize the code.

Unresolved questions:

When emoji list is open, press on "Settings" button and expect emoji list to hide - it does not.
How to better sort emojis in the list?

I'll be posting some more notes and questions inline.

/cc @bennyn, @gregor

maximbaz · 2017-04-07T22:20:09Z

app/script/view_model/ConversationInputEmojiViewModel.js

+    this.emoji_dict = undefined;
+    this.emoji_start_pos = -1;
+
+    $.get('/image/emoji.tsv', (text) => {


Maybe you can suggest a better way to load emojis, not by downloading the file like that?
I was thinking of creating JS file with one multiline constant and just including it in app.htm, but I'm not sure if that is any better.

This happens once per page load anyway.

If going with this solution we should use fetch instead of $.get..

Nice, I didn't even know about this 👍

maximbaz · 2017-04-07T22:27:19Z

app/style/content/conversation/conversation-input.less

+    padding: 4px 8px;
+
+    &.selected {
+      background: #2391d3;


I wanted to use user's color theme here, but I can't find a LESS variable that would give me the color. Did I miss it?
Saw z.entity.User.accent_color(), but definitely do not want to do this via JS.

background-color: @w-blue;

It's "hidden" a bit deeper: https://github.com/wireapp/wire-theme/blob/master/app/style/wire-theme.less#L29

cool 👍 but is there a way which will give me currently selected color, not always blue?
I stumbled upon some mixin .accent-background-color(), but I can't get it to work 😞

Right.. some magic will be needed with the themes here then! But it's a bit too late to figure out what exactly :)

maximbaz · 2017-04-07T22:53:18Z

app/script/view_model/ConversationInputEmojiViewModel.js

+        .map((emoji) => {
+          let [code, name] = emoji.split('\t');
+          let parsed_unicode_emoji = String.fromCodePoint.apply(null, code.split(','));
+          return `<div><span>${parsed_unicode_emoji}</span>${name}</div>`;


I know the project uses knockout.js and this whole creation of emoji list could have been done nicer. However I am quite hesitant to do such refactoring now, as this will add complexity to the whole feature.

This code is tested by many Surfingkeys users, we only need to test how well it integrates with Wire. Refactoring will be more risky, all of us would have to invest more time to get it right.

Considering this, do you think we can live with such code for emojis v1?

maximbaz · 2017-04-07T22:56:27Z

app/script/view_model/ConversationInputEmojiViewModel.js

+    this.emoji_list.find(`>div:nth(${new_selection})`).addClass('selected');
+  }
+
+  get_cursor_pixel_pos(input) {


This is some dark magic... 😉
but it works well, the emoji list is always positioned correctly from what I've tested so far.

lipis · 2017-04-07T23:05:46Z

app/script/view_model/ConversationInputEmojiViewModel.js

+  on_input_key_up(data, event) {
+    if (!this.suppress_key_up) {
+      let input = event.target;
+      let text = input.value || '';


lipis · 2017-04-07T23:08:24Z

Just awesome! 🎠

lipis · 2017-04-07T23:12:29Z

app/style/content/conversation/conversation-input.less

+
+    &.selected {
+      background: #2391d3;
+      color: white;


lipis · 2017-04-07T23:12:54Z

app/style/content/conversation/conversation-input.less

+  position: fixed;
+  display: block;
+  box-shadow: 0px 2px 10px rgba(0, 0, 0, 0.2);
+  background: white;


background-color: #fff

lipis · 2017-04-07T23:13:56Z

app/style/content/conversation/conversation-input.less

@@ -167,3 +167,27 @@
 .conversation-input-paste-cancel {
  margin-right: 32px;
 }
+
+.conversation-input-emoji-list {
+  z-index: 2147483000;


Maybe @z-index-level-7

lipis · 2017-04-07T23:20:23Z

app/script/view_model/ConversationInputEmojiViewModel.js

+      case z.util.KEYCODE.ENTER: {
+        let input = event.target;
+        let emoji = this.emoji_list.find('>div.selected>span').html();
+        let val = input.value;


lipis · 2017-04-07T23:56:53Z

app/script/view_model/ConversationInputEmojiViewModel.js

+  }
+
+  static get MIN_QUERY_LENGTH() {
+    return 2;


Magic numbers... not sure what would be the best approach.. maybe something like this (from SO):

const EMOJI_LIST_LENGTH = 5; const MIN_QUERY_LENGTH = 2; ... static get EMOJI_LIST_LENGTH() { return EMOJI_LIST_LENGTH; }

(even though smells like shadowing)

hm, I actually considered this static get to be the place where we define constants 😄 It does feel like an overkill to define a constant, that will be used to return from a constant 😉

My dream is to include this rule pretty soon: http://eslint.org/docs/rules/no-magic-numbers and while it feels like overkill I'm sure it will throw a warning.. any other ideas?

(obviously excluding 0s and 1s I guess from the rule)

I enabled it locally, and yes, it does throw a warning - will fix it then.

Actually, we don't need to create static getters, I created these constants not for exposing them outside of the module, but simply to make it clear what these numbers mean. I'll just leave the const definitions, and remove the rest.

lipis · 2017-04-07T23:58:27Z

app/style/content/conversation/conversation-input.less

+  z-index: @z-index-level-7;
+  position: fixed;
+  display: block;
+  box-shadow: 0px 2px 10px rgba(0, 0, 0, 0.2);


even though the designers will have the final say here.. at least style wise we use this format:rgba(0,0,0,.2)

lipis · 2017-04-08T00:12:24Z

Works pretty nicely.. but the suggestions are not clickable (nor changeable with arrow keys) is this by design?

(Chrome 59dev / macOS)

maximbaz · 2017-04-08T00:15:02Z

Not clickable - yes, but should be changeable with arrows up and down. Up / down to change, Enter to select, Esc to cancel. Hmm..

maximbaz · 2017-04-08T00:16:06Z

Heh, arrows break after clicking 😃 nice catch.
The focus has to stay in text input for arrows to work, so just click in the text input.

Hm, need to think how to handle this properly, as the keydown/keyup listener is registered only on the input box, and when you click elsewhere, the input is not the one who will receive keyup/keydown anymore...

lipis · 2017-04-08T00:19:09Z

And magically fix if you press Esc.. good luck :)

lipis · 2017-04-08T00:21:45Z

Also you forgot to /cc our Wire Ambassador of Emojis: @herrmannplatz 😬💩

maximbaz · 2017-04-08T01:04:36Z

I think a lot of people would find it natural to click on a emoji in the list, so I added this functionality... it looks a bit hacky, but works 😉

lipis · 2017-04-10T16:16:17Z

app/script/view_model/ConversationInputEmojiViewModel.js

@@ -127,9 +127,10 @@ z.ViewModel.ConversationInputEmojiViewModel = class ConversationInputEmojiViewMo
        this.emoji_list.find('.emoji:nth(0)').addClass('selected');

        const pos = this.get_cursor_pixel_pos(input);
-        const top = pos.top - this.emoji_list.height();
+        const top = pos.top - this.emoji_list.height() - 10;
+        const left = pos.left - 20;


magic :)

OFFSET_TOP/LEFT (or something like that)

maximbaz · 2017-04-10T16:20:12Z

So I found an interesting edge case, when emoij list is open, user can click elsewhere and the emoji list should hide itself. It didn't happen before, I extended the code so that the list is removed when edit is cancelled... but this doesn't cover all cases.

For example, enter :sm and while emoji list is open, click on Settings button.
Suggestions on how to handle this?

markesaaremets · 2017-04-10T16:34:06Z

@maximbaz

Thanks for the quick response!

12px are the names in Conversation, for example. Also the system messages will have the same style (12px Normal and Semibold) very soon.
16px emojis are also in conversation (if in the text), so two different sizes for emojis in general should be good.
Arrow we can keep for now, i'd say.

maximbaz · 2017-04-10T16:45:40Z

Got it, thanks, makes sense to be consistent. 👍

I'm looking at the sorting now, and I'm struggling a bit with how to make this better. The simple alphabetic sorting will make query like :sm compose a list of completely useless results, as 99% of time you are searching for some "smile" emojis when you are typing :sm, but instead with alphabetic sorting you are getting:

Small orange diamond
Small blue diamond
Small red triangle
Small red triangle down

The current code uses the order of emojis as they appear in emoji.tsv, so perhaps we could sort this file based on the "popularity", somehow. I did already put some very popular emojis in the top (the first 82 lines), but I didn't order the rest of the file.

Maybe you have some other ideas how to handle this nicely?

lipis · 2017-04-10T17:05:41Z

app/script/view_model/ConversationInputEmojiViewModel.js

+const QUERY_MIN_LENGTH = 2;
+
+const EMOJI_LIST_OFFSET_TOP = 10;
+const EMOJI_LIST_OFFSET_LEFT = 20;


sort them all by name.. no need to group them.

maximbaz · 2017-04-10T18:09:40Z

I tend to keep sorting as it is, based on the "popularity" defined by the ordering in the file. Subjectively I believe the "sunglasses" emoji is used more often than "sunny", that's why it comes first in the list.

When you see a specific emoji that should be above in the list, perhaps we could just bring it up in the file.

The cool engineering solution would be to keep a per-user statistics on what emojis this particular user is typing, and use this to further order the list. That way the fans of "Sunny" emoji would have it first in the list for :sun. Out of scope for this PR though 😜

Let me know if you agree.

I keep the list of unsolved questions in the first message here, besides the emoji ordering there's a bug with "Settings" button that I'm not sure yet how to solve, and apart from that I hope we are pretty close 🎉

lipis · 2017-04-10T18:13:46Z

I personally like this approach to sorting. Makes sense. Good stuff..

and no.. we are not going to personalize the usage of emojis :D

maximbaz · 2017-04-11T00:30:57Z

I've just had another look, and I think I might have figured out a way to fix the last issue, to hide emoji list on Settings / Collection buttons click. The trick I used is to subscribe to the CONTENT.SWITCH and hide emoji list when it occurs. That helped for the Collection button, but the problem was that click on Settings button did not generate this event... so now it does 🙂

I believe there are no unresolved issues anymore 🙂

markesaaremets · 2017-04-11T08:17:10Z

Text alignment seems to be dependent on emoji's width. It'd be a good to fix the width of left column (40px).
I'd disable the arrow for now
Name padding-right can be 16px

maximbaz · 2017-04-11T13:21:59Z

Nice catch with variable width, @makingthematrix! Fixed all the suggestions.

Since we don't have the arrow anymore, which offset do you think is the most pleasant to the eye? 🙂
With arrow we had 20px, but I think it's too much. As of now I changed it to 10px.

markesaaremets · 2017-04-11T13:23:39Z

@maximbaz Offset 8px ;)

markesaaremets · 2017-04-11T13:24:34Z

@bennyn, @lipis Design-wise, it's good to go.
Thank you, @maximbaz!

lipis · 2017-04-12T14:48:30Z

Boom!! Thanks again @maximbaz 🍻 🍺

bennycode · 2017-04-12T15:16:40Z

Hi @maximbaz, the whole @wireapp team is thankful beyond words.

Your pull request is a great addition to our app. It builds the foundation for emoji input support, one of the most requested features. Based on your contribution we can add an emoji picker and further input improvements...

Thanks for that!

Your friends at Wire. 🌝

maximbaz · 2017-04-12T15:31:33Z

Thank you guys for the support, reviews and suggestions, I'm glad we made it in the product 🎉

Initial implementation for emoji autocomplete

7b75363

maximbaz commented Apr 7, 2017

View reviewed changes

maximbaz added 2 commits April 8, 2017 00:37

Extract important constants

b833e5e

Make CoffeeLinter happy

3fff16c

maximbaz commented Apr 7, 2017

View reviewed changes

maximbaz mentioned this pull request Apr 7, 2017

Feature request: Emoji autocomplete #541

Closed

lipis reviewed Apr 7, 2017

View reviewed changes

Style improvements

a2365f7

lipis reviewed Apr 7, 2017

View reviewed changes

maximbaz added 2 commits April 8, 2017 01:22

Immutable rules the world

e90610d

Use 'fetch' to download emoji.tsv

3b3fa5a

lipis reviewed Apr 7, 2017

View reviewed changes

maximbaz added 3 commits April 8, 2017 02:24

Introduce constants to fix eslint 'no-magic-numbers'

18ef286

Fix rgba style

77beecb

Select emoji on mouse click

01fbcd5

maximbaz added 2 commits April 8, 2017 03:11

Remove static getters for constants in the class

7a234ee

Cleanup

c52c490

lipis reviewed Apr 10, 2017

View reviewed changes

Hide emoji list when edit is cancelled

2b2717f

Introduce offset constants for bubble-style emoji list

e542923

Make emoji searching case insensitive

19f16ca

lipis reviewed Apr 10, 2017

View reviewed changes

Sort constants

c143f6b

Close emoji list on Settings / Collection buttons click

0566487

maximbaz force-pushed the 541-emoji-autocomplete branch from 5f726ac to 0566487 Compare April 11, 2017 00:37

Fix text alignment regardless of emoji width

6c0d2d1

Remove bubble arrow

20a81fe

maximbaz and others added 3 commits April 11, 2017 15:25

Adjust offsets

2b7f913

Merge branch 'dev' into 541-emoji-autocomplete

5442ca1

New linter rules :)

0710d6c

lipis approved these changes Apr 12, 2017

View reviewed changes

lipis merged commit acbbd2b into wireapp:dev Apr 12, 2017

maximbaz mentioned this pull request Apr 24, 2017

Sort emoji in autocomplete #1102

Merged

lokesh-krishna mentioned this pull request May 4, 2017

Emoji Typeahead signalapp/Signal-Desktop#197

Closed

Emoji autocomplete 🎉 ⚡ #1038

Emoji autocomplete 🎉 ⚡ #1038

Conversation

maximbaz commented Apr 7, 2017 • edited Loading

maximbaz Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipis commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipis Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipis Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

lipis commented Apr 8, 2017

maximbaz commented Apr 8, 2017

maximbaz commented Apr 8, 2017 • edited Loading

lipis commented Apr 8, 2017

lipis commented Apr 8, 2017

maximbaz commented Apr 8, 2017

Choose a reason for hiding this comment

maximbaz commented Apr 10, 2017

markesaaremets commented Apr 10, 2017

maximbaz commented Apr 10, 2017

Choose a reason for hiding this comment

maximbaz commented Apr 10, 2017

lipis commented Apr 10, 2017

maximbaz commented Apr 11, 2017

markesaaremets commented Apr 11, 2017

maximbaz commented Apr 11, 2017

markesaaremets commented Apr 11, 2017

markesaaremets commented Apr 11, 2017 • edited Loading

lipis commented Apr 12, 2017

bennycode commented Apr 12, 2017

maximbaz commented Apr 12, 2017

maximbaz commented Apr 7, 2017 •

edited

Loading

maximbaz Apr 7, 2017 •

edited

Loading

maximbaz Apr 7, 2017 •

edited

Loading

lipis Apr 8, 2017 •

edited

Loading

lipis Apr 7, 2017 •

edited

Loading

maximbaz commented Apr 8, 2017 •

edited

Loading

markesaaremets commented Apr 11, 2017 •

edited

Loading