-
Notifications
You must be signed in to change notification settings - Fork 1
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
Upgrade Emojibase and Add emoticon variations #21
Conversation
…langleyd/add_emoticon_permutations
…langleyd/add_emoticon_permutations
Dropping in uninvited to ask an important question!
Does this work per platform? Apple supports 15.1 and I'd love to get the 🙂 |
@pixlwave yea we could probably load it via an |
Amazing, thank you! 🙌 |
@bmarty We've split out the max emoji version per platform as per the PR description. Would you(Android) like add emoji 15.1 support similar to iOS? (Web needs to stay on 15.0 for now). |
src/emoji.ts
Outdated
for (const [versionString, emojis] of Object.entries(VERSIONS)) { | ||
const version = parseFloat(versionString); | ||
emojis.forEach((emoji) => EMOJI_TO_VERSION.set(emoji, version)); | ||
} |
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.
A comment is welcomed to explain why we need it
//flatten, in case `emoji.emoticon` is an array | ||
.flat() | ||
.flatMap((x) => generateEmoticonPermutations(x)); | ||
emoticons.forEach((x) => EMOTICON_TO_EMOJI.set(x, emoji)); |
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.
It's feel wrong to me to mutate a map/array when doing a map another array.
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.
Yes, true I found that strange also, I wasn't too comfortable making those logic changes before as there wasn't much tests but we have that now :). Will move the logic that isn't related to the map out to a foreach so it's a bit clearer.
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.
Looks great from the iOS perspective, thanks for the update 👏
Confirmed with Android team they want to move to 15.1. |
What's in this PR
generateEmoticonPermutations
to generate variations of plain text emoticons(e.g. with or without nose:)
:-)
).MAX_EMOJI_VERSION_WEB
to filter out newer emojis from inclusion as web is not ready to upgrade yet (it needs to be upgraded in unison withtwemoji
(which is awkward and beyond the scope of this PR)MAX_EMOJI_VERSION_ANDROID
andMAX_EMOJI_VERSION_IOS
to the script that generates theemojibase.json
for those platforms so that they can move forward.