-
-
Notifications
You must be signed in to change notification settings - Fork 567
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
fix(suggestion): allow case-insensitive emoji suggestion #2565
fix(suggestion): allow case-insensitive emoji suggestion #2565
Conversation
Run & review this pull request in StackBlitz Codeflow. |
✅ Deploy Preview for elk-docs canceled.
|
✅ Deploy Preview for elk-zone ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
composables/tiptap/suggestion.ts
Outdated
.then(data => Object.values(data.emojis).filter(({ id }) => id.toLowerCase().startsWith(query.toLowerCase()))) | ||
|
||
const customEmojis: CustomEmoji[] = currentCustomEmojis.value.emojis | ||
.filter(emoji => emoji.shortcode.startsWith(query)) | ||
.filter(emoji => emoji.shortcode.toLowerCase().startsWith(query.toLowerCase())) |
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.
We should cache query.toLowerCase()
to avoid recomputing it during the loops.
But I think we should do the same with shortcode
and id
, and cache the result of import('@emoji-mart/data')
with the data as we needed in lowercase. This will also speed up things
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.
OK, I removed multiple calls of toLowerCase()
for query
.
Also, I used useAsyncData
to cache emojimart data import. Is this the right approach for caching?
Regarding making id
and shortcode
lowercase, there is an issue. If we lower the shortcode beforehand, we cannot show the original shortcode for users like the below.
This is problematic since some custom emoji use camelcase naming like :thisIsCamelCaseEmojiName:
. The lowercased name :thisiscamelcaseemojiname:
is a bit difficult to read.
Maybe we can keep as is since I assume it's not so heavy computation to call toLowerCase()
as many times as emoji counts.
c8a6c46
to
c6c8004
Compare
c6c8004
to
24df5ed
Compare
fix #2552
Currently, we're simply comparing strings with
startsWith()
so users have to type the exact same case as its emoji id or shortcode.This change supports a case-insensitive search despite either id/shortcode or user query containing any upper/lower cases.
Also, it looks like Mastodon's custom emoji search doesn't distinguish cases of shortcode: mastodon/mastodon#15738