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.

Sign up for GitHub

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

Jump to bottom

Fix: Size of the single emoji in chat along with updated Emoji Regex #5039

Merged

marcaaron merged 18 commits into Expensify:main from mananjadhav:fix/single-emoji-size

Sep 13, 2021

Collaborator

mananjadhav commented Sep 2, 2021 •

edited

Loading

@deetergp I've updated my changes with the regex changes and a unit test to verify all the emojis in our dataset.

Details

Changed isSingleEmoji function
Replaced Emoji Regex

Fixed Issues

Tests

Tested different variants of the Emojis with single length and surrogate pairs
Tested emojis with skin tone.
Tested with composite emojis such as Family
Tested text-based Emojis

QA Steps

Go to any Chat
Enter one emoji per message.
Size of the Emoji should be larger similar to other apps like Whatsapp, etc.
It should work for all variations.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

mananjadhav added 3 commits

September 3, 2021 04:26


          fix(emoji-size-in-chat): Moved regex to a different function

f0273ce


          fix(emoji-size-in-chat): Convert emoji to Unicode and compare

41d5ecf


          fix(emoji-size-in-chat): Replaced Emoji Regex

800e002

Collaborator Author

mananjadhav commented Sep 2, 2021

@deetergp I've added my changes and I am currently testing the coverage of the regex. The initial results look good with few exceptions.

mananjadhav added 5 commits

September 8, 2021 03:47


          fix(emoji-size-in-chat): Handled skin-tone support

8fe203d


          fix(emoji-size-in-chat): wip: Added test for emoji regex

1da613a


          fix(emoji-size-in-chat): Update emoji regex test and pending emoji range

87a29b2


          fix(emoji-size-in-chat): Added skintone emoji in test

1f6354b


          fix(emoji-size-in-chat): Hardcoded emoji variants to test against the…

c07557b

… regex

mananjadhav changed the title ~~Fix single emoji size in chat~~ Fix: Size of the single emoji in chat along with updated Emoji Regex


          fix(emoji-size-in-chat): Removed unused import

cb3a584

mananjadhav marked this pull request as ready for review

September 8, 2021 12:32

mananjadhav requested a review from a team as a code owner

September 8, 2021 12:32

MelvinBot requested review from marcaaron and removed request for a team

September 8, 2021 12:32

Collaborator Author

mananjadhav commented Sep 8, 2021

@deetergp PR updated. Please review.

deetergp requested changes

View reviewed changes

src/libs/Emoji/getEmojiUnicode.js Outdated Show resolved Hide resolved

src/CONST.js Outdated Show resolved Hide resolved

src/libs/Emoji/getEmojiUnicode.js Outdated Show resolved Hide resolved

mananjadhav added 2 commits

September 9, 2021 05:42


          fix(emoji-size-in-chat): Replace function with inline regex

c6e1d9a


          fix(emoji-size-in-chat): Merged function into one

09a56b5

Collaborator Author

mananjadhav commented Sep 9, 2021

@deetergp PR updated

deetergp previously approved these changes

View reviewed changes

Contributor

deetergp left a comment

Looks good, thanks for the changes!

marcaaron requested changes

View reviewed changes

src/libs/Emoji/getEmojiUnicode.js Outdated

+               * Get the unicode code of an emoji in base 16.
+               *
+               * @name emojiUnicode
+               * @function

Contributor

marcaaron Sep 10, 2021

I think we can make these docs more consistent with the others in our app. Notably, @name and @function + the repeated name before the description. We won't find these anywhere so I think better to not use them.

Collaborator Author

mananjadhav Sep 10, 2021

Okay noted.

src/libs/Emoji/getEmojiUnicode.js Outdated

		@@ -0,0 +1,38 @@
		/* eslint radix: ["error", "as-needed"] */

Contributor

marcaaron Sep 10, 2021

What are we silencing here?

Collaborator Author

mananjadhav Sep 10, 2021

parseInt also accepts a second param radix, which I've marked as-needed.

Contributor

marcaaron Sep 10, 2021

Why not just add the radix?

src/libs/Emoji/getEmojiUnicode.js Outdated

+                  }
+                  if (input.length === 1) {
+                      return input.charCodeAt(0).toString().split(' ').map(val => parseInt(val).toString(16))

Contributor

marcaaron Sep 10, 2021

Please use _.map()

src/libs/Emoji/getEmojiUnicode.js Outdated

+                  for (let i = 0; i < input.length; i++) {
+                      if (input.charCodeAt(i) >= 0xd800 && input.charCodeAt(i) <= 0xdbff) { // high surrogate
+                          if (input.charCodeAt(i + 1) >= 0xdc00 && input.charCodeAt(i + 1) <= 0xdfff) {
+                              // low surrogate

Contributor

marcaaron Sep 10, 2021

What is a surrogate?

Collaborator Author

mananjadhav Sep 10, 2021

Some of the emojis are stored as two Unicodes. Hence we call each of the Unicode as high surrogate and low surrogate

https://thekevinscott.com/emojis-in-javascript/

Contributor

marcaaron Sep 10, 2021

Got it thanks. I'm not really familiar with this language - could we maybe add a comment that briefly explains what we mean when we refer to a "surrogate"?

src/libs/Emoji/getEmojiUnicode.js Outdated

+                          pairs.push(input.charCodeAt(i));
+                      }
+                  }
+                  return pairs.map(val => parseInt(val).toString(16)).join(' ');

Contributor

marcaaron Sep 10, 2021

_.map()

src/libs/ValidationUtils.js Outdated

@@ @@ -2,6 +2,8 @@ import moment from 'moment'; @@
               import CONST from '../CONST';
               import {showBankAccountFormValidationError, showBankAccountErrorModal} from './actions/BankAccounts';
               import {translateLocal} from './translate';
+              import getEmojiUnicode from './Emoji/getEmojiUnicode';
+              import trimEmojiUnicode from './Emoji/trimEmojiUnicode';

Contributor

marcaaron Sep 10, 2021

Probably these could be combined into a single file called EmojiUtils.js or something? We don't enforce one file per method.

Collaborator Author

mananjadhav Sep 10, 2021

Okay noted

src/libs/ValidationUtils.js Outdated

-                  return message.length === matchedEmoji.length;
+                  const matchedUnicode = getEmojiUnicode(matchedEmoji);
+                  const currentMessageUnicode = trimEmojiUnicode(getEmojiUnicode(message));
+                  return matchedUnicode === currentMessageUnicode;

Contributor

marcaaron Sep 10, 2021

I'm not sure when isSingleEmoji() was added - but it doesn't belongs in this file. ValidationUtils is for validating user input into a form field not to check if something is an emoji. Let's please move it somewhere else.

Collaborator Author

mananjadhav Sep 10, 2021

Okay should I move it to EmojiUtils, that I'll create for the above functions?

Contributor

marcaaron Sep 10, 2021

That works for me! Thanks!

tests/unit/EmojiRegexTest.js Show resolved Hide resolved

mananjadhav added 6 commits

September 10, 2021 23:24


          Merge branch 'main' of https://github.com/mananjadhav/App into fix/si…

73655c7

…ngle-emoji-size


          fix(emoji-size-in-chat): Merged all Emoji functions into EmojiUtils

031191e


          fix(emoji-size-in-chat): Added comments on surrogate and changed to _…

b3322b6

….map


          fix(emoji-size-in-chat): Updated test documentation

09babe0


          Merge branch 'main' of https://github.com/mananjadhav/App into fix/si…

5d75570

…ngle-emoji-size


          fix(emoji-size-in-chat): Added additional skull emoji from previous r…

22ca1d7

…aised bug

mananjadhav dismissed deetergp’s stale review via

22ca1d7

September 10, 2021 20:48

Collaborator Author

mananjadhav commented Sep 10, 2021

@marcaaron PR updated.

mananjadhav requested a review from marcaaron

September 10, 2021 20:49

deetergp previously approved these changes

View reviewed changes

Contributor

deetergp left a comment

Looking good, thanks for the changes!

marcaaron reviewed

View reviewed changes

src/libs/EmojiUtils.js Outdated

		@@ -0,0 +1,76 @@
		/* eslint radix: ["error", "as-needed"] */

Contributor

marcaaron Sep 10, 2021

I think we can just add the radix here.. is it something other than 10? That's what we do everywhere else.

Collaborator Author

mananjadhav Sep 11, 2021 •

edited

Loading

Well, with Emojis you can’t be sure that the you’ll get an integer with base 10. This could be because copy pasting emojis from other apps. If you see the code, we are splitting a string and it could start with 0x too.

Added radix.

marcaaron assigned mananjadhav


          fix(emoji-size-in-chat): Added radix to parseInt

52d9827

mananjadhav dismissed deetergp’s stale review via

52d9827

September 11, 2021 06:59

mananjadhav requested a review from marcaaron

September 11, 2021 07:00

marcaaron approved these changes

View reviewed changes

marcaaron merged commit 04bf0ce into Expensify:main

OSBotify mentioned this pull request

Deploy Checklist: New Expensify 2021-09-13 #5224

Closed

14 tasks

Contributor

OSBotify commented Sep 13, 2021

🚀 Deployed to staging by @marcaaron in version: 1.0.97-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

botify commented Sep 15, 2021

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-22. 🎊

Contributor

OSBotify commented Sep 15, 2021

🚀 Deployed to production by @francoisl in version: 1.0.98-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet