Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Upgrade emojibase and twemoji #7286

Merged
merged 13 commits into from
Mar 23, 2022

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented Dec 5, 2021

Type: task


Fixes emoji for :D and allows emoji for capital :P


This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://61e5413fc5b56620e97d4035--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@github-actions github-actions bot added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Dec 5, 2021
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner marked this pull request as ready for review December 5, 2021 08:33
@SimonBrandner SimonBrandner requested a review from a team as a code owner December 5, 2021 08:33
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

So I don't think we can merge this because Twemoji hasn't been updated to Emoji 14 yet https://github.com/twitter/twemoji/releases. But LGTM otherwise.

@t3chguy t3chguy added the X-Blocked The PR cannot move forward in any capacity until an action is made label Dec 6, 2021
SimonBrandner and others added 2 commits December 11, 2021 13:44
Co-authored-by: Tulir Asokan <tulir@maunium.net>
@tulir
Copy link
Member

tulir commented Dec 13, 2021

This seems to also break autocompleting emojis with non-prefix strings, e.g. 🧐 (:face_with_monocle:) by typing :monocle. I have no idea why/how though, maybe something to do with the emojibase-regex update

@tulir
Copy link
Member

tulir commented Dec 24, 2021

I figured out the problem, the fields used by emoji autocompletion aren't type-checked:

diff --git a/src/autocomplete/EmojiProvider.tsx b/src/autocomplete/EmojiProvider.tsx
index 960b0f9475..55d58a8953 100644
--- a/src/autocomplete/EmojiProvider.tsx
+++ b/src/autocomplete/EmojiProvider.tsx
@@ -75,7 +75,7 @@ export default class EmojiProvider extends AutocompleteProvider {
             shouldMatchWordsOnly: false,
         });
         this.nameMatcher = new QueryMatcher(SORTED_EMOJI, {
-            keys: ['emoji.annotation'],
+            keys: ['emoji.label'],
             // For removing punctuation
             shouldMatchWordsOnly: true,
         });

@SimonBrandner SimonBrandner self-assigned this Dec 24, 2021
@SimonBrandner SimonBrandner removed the X-Blocked The PR cannot move forward in any capacity until an action is made label Dec 25, 2021
@SimonBrandner SimonBrandner removed their assignment Dec 25, 2021
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Wonder if we can prove the typing here using something like https://stackoverflow.com/questions/45415436/nested-keyof-object-paths-using-dot-notation

@SimonBrandner SimonBrandner added the X-Blocked The PR cannot move forward in any capacity until an action is made label Jan 5, 2022
@t3chguy
Copy link
Member

t3chguy commented Jan 27, 2022

Blocked on twitter/twemoji#515

@SimonBrandner
Copy link
Contributor Author

Twemoji has now been upgraded, so after a review this should be ready to go

@SimonBrandner SimonBrandner removed the X-Blocked The PR cannot move forward in any capacity until an action is made label Mar 23, 2022
@SimonBrandner SimonBrandner changed the title Upgrade emojibase Upgrade emojibase and twemoji Mar 23, 2022
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks good to me, just pending an oob manual test of the new Twemoji fonts

@SimonBrandner SimonBrandner added X-Blocked The PR cannot move forward in any capacity until an action is made and removed X-Blocked The PR cannot move forward in any capacity until an action is made labels Mar 23, 2022
@SimonBrandner
Copy link
Contributor Author

After going through the emoji, it seems nothing is broken

@t3chguy t3chguy enabled auto-merge (squash) March 23, 2022 16:33
@t3chguy t3chguy merged commit 3534e9b into matrix-org:develop Mar 23, 2022
@SimonBrandner SimonBrandner deleted the task/upgrade-emojibase branch March 23, 2022 17:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants