Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add emoji autocompletion #1474

Merged
merged 9 commits into from
Dec 29, 2021
Merged

Add emoji autocompletion #1474

merged 9 commits into from
Dec 29, 2021

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Oct 15, 2020

image

@skjnldsv skjnldsv added 2. developing Work in progress feature: rich-contenteditable Related to the rich-contenteditable components labels Oct 15, 2020
@skjnldsv skjnldsv self-assigned this Oct 15, 2020
@PVince81

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@PVince81

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Mar 8, 2021

what's missing ?

Joke aside, it's working, just need to be polished for the ui (not show the whole list, maybe show the shortcut? This is also not translated. Anyone can take over, this is mostly for talk, so I'll let you folks decide what needs to be done. cc @jancborchardt

You can all test it in the deploy preview https://deploy-preview-1474--nextcloud-vue-components.netlify.app/

@skjnldsv skjnldsv marked this pull request as ready for review March 8, 2021 16:28
@simonspa
Copy link

Concerning the UI, this is how Mattermost looks like, something I find very well working for emoji autocompletion:

image
image

A list of matching emojis, with a scrollbar if too many, and the emoji "name" / code listed behind the emoji.

@skjnldsv
Copy link
Contributor Author

@jancborchardt are you ok with this design (see comment above) ?

@PVince81
Copy link
Contributor

PVince81 commented May 12, 2021

  • BUG: styles seems broken currently, might need a rebase first:

image

also, when typing just ":s" it's very slow/sluggish in my Firefox until I type more letters.
probably this is because it renders too many results until it gets narrowed down, maybe later need to add some virtual scrolling there

@PVince81
Copy link
Contributor

PVince81 commented May 12, 2021

  • BUG? doesn't autocomplete / replace ascii emojis like ":-)" (could be something for later or needs a DB extension)
  • BUG: hitting enter key after typing ":-)" doesn't go to the next line, but after other words it does

@skjnldsv
Copy link
Contributor Author

Please take over @PVince81 :)

@skjnldsv skjnldsv removed their assignment May 12, 2021
@mejo- mejo- force-pushed the feat/emoji branch 4 times, most recently from c128781 to 3de5082 Compare December 2, 2021 16:05
@mejo-
Copy link
Contributor

mejo- commented Dec 2, 2021

I put some time into this PR and think that it's in a state to be reviewed now 😊

Instead of building another own emoji data set, the existing dataset by emoji-mart-fast is used. Same for emoji lookup/search functions. This brings unified dataset, defaults and recently used logic between EmojiPicker and emoji autocompletion.

The number of displayed results is limited to five (Github etc. do the same).

I also went with implementing the emojiSearch and addRecent in a seperate functions file, so it can be used again by others (e.g. the Text app, see nextcloud/text#1961).

That's how it looks like at the moment:

recording

Looking forward to your thoughts @skjnldsv @PVince81 @jancborchardt @nimishavijay 😊

@mejo- mejo- added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 2, 2021
@mejo- mejo- self-assigned this Dec 2, 2021
@mejo-
Copy link
Contributor

mejo- commented Dec 27, 2021

* How will this work with other languages?

Unfortunately there's no support for translated emoji names in emoji-mart-vue yet. I'd suggest to discuss this in a separate follow-up issue/PR.

* Purely visual nitpicking, but a couple of pixels of space between the emoji and the text might be a good idea

Agreed. I now added padding-right: 8px to the emoji in the dropdown. See the updated screenshots: #1474 (comment)

* I'm also thinking if it is a good idea to have a scrollbar instead of a hard limit of 5 results, because it might make it easier to find results that are buried down.
  For example, I call this white_check_mark  a tick mark, so if I type `:white_tick` then white_check_mark shows up because it is the top result. But when I type `:tick` the emoji doesn't show up even though it is matched, because it is below the top 5 matched results. Having it scrollable would allow me to find more results.
  Github limits the results to only 5, so I'm thinking if it is such a big deal after all thinking

Not sure about this. We definitely need a limit here, otherwise we would list more than 3000 emojis without a query string. I like the short list of five results and that's what I discovered at Github and Gitlab. Element (the Matrix client) has a limit of 20 emojis (with a scrollbar), Rocketchat lists 10 emojis (with a scrollbar). Notions lists a lot, but just the emojis (without their name) in a table.

Here's how emoji autocompletion would look with a limit of 20 results:

recording

Let me know, which one you prefer 😊

@mejo- mejo- requested a review from marcoambrosini December 27, 2021 10:43
Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I'd also like to have the ability to scroll and In my opinion 10 results are enough.

Also, we could round the results focus feedback a bit more (from 5 to 8px) and increase the padding of the container to 6 or 8px (screenshot is 8)
Screenshot 2021-12-28 at 10 35 30

Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

But since this is already super awesome I'm just going to approve, these comments are just tiny details :)

@mejo-
Copy link
Contributor

mejo- commented Dec 28, 2021

I'd also like to have the ability to scroll and In my opinion 10 results are enough.

Ok, I'll wait for one or two more opinions and raise the limit to 10 if nobody opposed 😊

Also, we could round the results focus feedback a bit more (from 5 to 8px) and increase the padding of the container to 6 or 8px (screenshot is 8) Screenshot 2021-12-28 at 10 35 30

Agreed! I don't mind to quickly change this if others agree as well 😊

@jancborchardt
Copy link
Contributor

Looks very nice! :)

  • Agree with @marcoambrosini’s feedback on both limit and padding
  • When the container is scrollable, the height should cut off at the half of an entry rather than at the full entry cause that communicates better that there are more entries. :)
  • Currently the width of the container changes based on the length of the text inside – we should make it a set width always, then it jumps around less.

@mejo-
Copy link
Contributor

mejo- commented Dec 28, 2021

* Currently the width of the container changes based on the length of the text inside – we should make it a set width always, then it jumps around less.

Good point 😊 What would a good width be? Currently it has min-width: 180px and max-width: 300px. Changing to min-width: 300px looks a bit too wide, especially on mobile and when searching for short names:

2021-12-28T14:51:20,252681508+01:00

Maybe 250px is a good fixed width:

2021-12-28T14:53:06,647980626+01:00

@mejo-
Copy link
Contributor

mejo- commented Dec 28, 2021

Ok, did all the requested changes:

  • Limit to 10 results
  • border-radius: 8px
  • Fixed width of 300px for container
  • padding: 8px for container
  • Show 5 entries and a half to show scrollbar

Screenshots (and gif) updated: #1474 (comment)

@jancborchardt
Copy link
Contributor

Very nice! From the gif the width seems a bit mich for most of the emojis, if it's 200px (and ellipsized text) then all good! 👍

@mejo-
Copy link
Contributor

mejo- commented Dec 28, 2021

Very nice! From the gif the width seems a bit mich for most of the emojis, if it's 200px (and ellipsized text) then all good! +1

Ok, done: #1474 (comment)

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Nice! :)

@@ -36,7 +36,7 @@ Note you need to register the [tooltip directive](https://nextcloud-vue-componen
:auto-complete="autoComplete"
:maxlength="100"
:user-data="userData"
placeholder="Try mentioning the user Test01"
placeholder="Try mentioning user @Test01 or inserting emoji :smile"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
placeholder="Try mentioning user @Test01 or inserting emoji :smile"
placeholder="Try mentioning user @Test01 or inserting emoji :smile:"

Typo?

@@ -46,7 +46,7 @@ Note you need to register the [tooltip directive](https://nextcloud-vue-componen
:maxlength="400"
:multiline="true"
:user-data="userData"
placeholder="Try mentioning the user Test01"
placeholder="Try mentioning user @Test01 or inserting emoji :smile"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
placeholder="Try mentioning user @Test01 or inserting emoji :smile"
placeholder="Try mentioning user @Test01 or inserting emoji :smile:"

Typo?

Copy link
Contributor

Choose a reason for hiding this comment

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

I decided not to append a colon on purpose. The trailing colon is not needed for autocompletion and makes it harder to read in my opinion. It's more that the prepending colon is the trigger for autocompletion, and then you type your search query (e.g. "smile").

I agree that this might change when we implement automatic conversion of :smile: to an emoji, but I would say that's out of scope here 😊

Copy link
Contributor Author

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

So, autocompletion is one thing, but we're not converting the data in emoji when rendering.
If I type the text :smile:, there will be no emoji displayed, right?
I guess that would be a nice second step for the RichEdior :)

@mejo- mejo- merged commit 62900ec into master Dec 29, 2021
@mejo- mejo- deleted the feat/emoji branch December 29, 2021 12:37
max-nextcloud pushed a commit to nextcloud/text that referenced this pull request Jan 20, 2022
This brings unified emoji datasource,defaults and recents for
EmojiPicker and the emoji autocompletion feature.

This depends on nextcloud-libraries/nextcloud-vue#1474

Signed-off-by: Jonas Meurer <[email protected]>
mejo- added a commit to nextcloud/text that referenced this pull request Feb 16, 2022
Adds emoji autocompletion by typing `:<search_string>`.

It utilizes the emoji functions from nextcloud-vue[1] and the TipTap
suggestions extension.

[1] nextcloud-libraries/nextcloud-vue#1474

Signed-off-by: Jonas Meurer <[email protected]>
mejo- added a commit to nextcloud/text that referenced this pull request Feb 22, 2022
Adds emoji autocompletion by typing `:<search_string>`.

It utilizes the emoji functions from nextcloud-vue[1] and the TipTap
suggestions extension.

[1] nextcloud-libraries/nextcloud-vue#1474

Signed-off-by: Jonas Meurer <[email protected]>
mejo- added a commit to nextcloud/text that referenced this pull request Feb 22, 2022
Adds emoji autocompletion by typing `:<search_string>`.

It utilizes the emoji functions from nextcloud-vue[1] and the TipTap
suggestions extension.

[1] nextcloud-libraries/nextcloud-vue#1474

Signed-off-by: Jonas <[email protected]>
max-nextcloud pushed a commit to nextcloud/text that referenced this pull request Mar 1, 2022
Adds emoji autocompletion by typing `:<search_string>`.

It utilizes the emoji functions from nextcloud-vue[1] and the TipTap
suggestions extension.

[1] nextcloud-libraries/nextcloud-vue#1474

Signed-off-by: Jonas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews feature: rich-contenteditable Related to the rich-contenteditable components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants