Make emojis larger when multiple are sent at the same time #8896

eVoloshchak · 2022-05-06T17:18:03Z

Details

Previously emojis in messages were larger than text only if you send message consisting of only one emoji. This PR ensures that if message consists only of emojis, they are displayed larger that text, regardless of the length.

Fixed Issues

#8867

Tests

Send a message that contains only emojis
Verify that emojis in this message are larger that the text (27pt)
Send a message that contains both text and emojis
Verify that emojis and text are small and have the same font size

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Send a message that contains only emojis
Verify that emojis in this message are larger that the text (27pt)
Send a message that contains both text and emojis
Verify that emojis and text are small and have the same font size

Screenshots

Web

Mobile Web

Desktop

I'm unable to launch the desktop app due to this bug. It is not present if you downgrade to certain node and npm versions, but since I use MacinCloud, I'm unable to do so. So if someone with a mac could test it and upload a video, I would highly appreciate it.

iOS

Android

src/libs/EmojiUtils.js

eVoloshchak · 2022-05-09T17:39:17Z

Updated

src/libs/EmojiUtils.js

src/styles/styles.js

Santhosh-Sellavel · 2022-05-10T16:27:22Z

Even if there are spaces in between Emojis should they be small?

@eVoloshchak @Julesssss

Santhosh-Sellavel · 2022-05-10T16:29:31Z

PR Reviewer Checklist

I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

Santhosh-Sellavel · 2022-05-10T16:31:58Z

@eVoloshchak

I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)

Add failure steps i.e which is adding text along with emoji will make them smaller

Julesssss · 2022-05-10T16:33:09Z

Even if there are spaces in between Emojis should they be small?

Hmm, nice edgecase! I missed that.

Part of me thinks that yes a space character could be an exception -- but it's a bit of a rabbit hole as I'm sure we could think of other exceptions too. Ultimately I don't think it matters too much either way

Julesssss

Tested well for me on all platforms.

Julesssss · 2022-05-10T16:38:04Z

CC @tgolen who may want to review, as you have essentially already reviewed

Santhosh-Sellavel · 2022-05-10T16:38:53Z

Even if there are spaces in between Emojis should they be small?

Hmm, nice edgecase! I missed that.

Part of me thinks that yes a space character could be an exception -- but it's a bit of a rabbit hole as I'm sure we could think of other exceptions too. Ultimately I don't think it matters too much either way

This could be plausible regressions just checking with you.

I could see only whitespaces and a newLine,

Santhosh-Sellavel

I left a few edge cases here #8896 (comment).

Otherwise Looks good & test's well for me!
cc: @Julesssss

Santhosh-Sellavel · 2022-05-10T16:40:17Z

src/styles/styles.js

        fontSize: variables.fontSizeSingleEmoji,
        lineHeight: variables.fontSizeSingleEmojiHeight,


NAB! these variables could be renamed too.

Julesssss · 2022-05-10T16:50:29Z

On second thoughts, perhaps we should handle newline and space... Ignore my previous reasoning.

Finally, I see no nociable performance hit on a real/emulated Android device.

Santhosh-Sellavel

@eVoloshchak Can you handle these cases thanks! #8896 (comment)

eVoloshchak · 2022-05-10T17:54:36Z

@eVoloshchak Can you handle these cases thanks! [#8896 (comment)]

Thats a good catch
I've commited fix for emojis with spaces between them.

I have a solution for the case with line breaks, but would like your input on it before i push it:
Problem: when we have a line break between characters, there is a <br /> tag between them. That means that condition here is false (since fragment.text does not include html tags) and we RenderHTML rather than rendering Text, which means EmojiUtils.containsOnlyEmojis will never get called in this case.

Proposed solution:

Determine if the only difference between fragment.text and fragment.html is a <br /> tag
If if is, replace <br /> tag with line break ('/n')
Assign newly created string to both fragment.text and fragment.html, since they are effectively the same
Ignore line breaks in containsOnlyEmojis function

In practice, it means adding the following before this line:

  const differByLineBreaksOnly = props.fragment.html.replaceAll('<br />', ' ') === props.fragment.text;
  if (differByLineBreaksOnly) {
      props.fragment.text = props.fragment.html = props.fragment.html.replaceAll('<br />', '\n');
  }

What do you think?

tgolen · 2022-05-10T20:02:16Z

I think I'm OK with that fix for the newlines. Nice that the whitespace idea was suggested.

tgolen

Thank you!

tgolen · 2022-05-12T15:01:38Z

all you @Santhosh-Sellavel

Santhosh-Sellavel · 2022-05-12T16:44:37Z

Yeah will check this today.

Julesssss

Re-approving. Tests well 👍

Santhosh-Sellavel

Just a small change can we clean this up, thanks!

Santhosh-Sellavel · 2022-05-12T17:38:53Z

src/libs/EmojiUtils.js

+
+    // Emojis are stored as multiple characters, so we're using spread operator
+    // to iterate over the actual emojis, not just characters that compose them
+    const messageCodes = _.filter(_.map([...message.replace(/ /g, '').replaceAll('\n', '')],


This is not DRY, message.replace(/ /g, '').replaceAll('\n', '') computing this twice.

This is not DRY, message.replace(/ /g, '').replaceAll('\n', '') computing this twice.

All done

Santhosh-Sellavel

Looks good to me, tests well!

cc: @tgolen @Julesssss

Julesssss

Thanks all, looks good to me 👍

Santhosh-Sellavel · 2022-05-15T17:14:49Z

I think we are good to merge this cc: @tgolen

OSBotify · 2022-05-16T15:44:13Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-05-16T18:21:28Z

🚀 Deployed to staging by @tgolen in version: 1.1.61-0 🚀

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

OSBotify · 2022-05-17T16:59:15Z

🚀 Deployed to production by @AndrewGable in version: 1.1.61-3 🚀

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

marcaaron · 2022-05-17T23:55:34Z

Interesting FYI that these changes appear to be breaking mobile web for me on iOS simulator running iOS 13 - my device running iOS 15 is not affected. Pretty sure we are not tracking support for mobile web on all iOS versions so unsure if this counts as a "regression" - but gonna fix it.

marcaaron · 2022-05-17T23:56:05Z

replaceAll() seems to not be available in Safari iOS 13.

eVoloshchak · 2022-05-18T11:38:22Z

@marcaaron, that's a good catch, thank you!
replaceAll() is indeed unavailable on iOS 13.3 and below, so we can just use replace() with //g regex instead.
I submitted a PR that fixes this: #9063

Santhosh-Sellavel · 2022-05-18T17:31:09Z

@marcaaron I think we should capture this somewhere to avoid this happening again.

marcaaron · 2022-05-18T19:07:52Z

Thanks, I'm working on a fix already so feel free to close your PR @eVoloshchak.

@marcaaron I think we should capture this somewhere to avoid this happening again.

One solution I can think of is to...

Create a change in expensify-common Str.replaceAll() and use a polyfill there
Enforce this method is used with a lint rule that prohibits using replaceAll() and prefers using Str.replaceAll()

Santhosh-Sellavel · 2022-05-18T19:14:10Z

One solution I can think of is to...

Create a change in expensify-common Str.replaceAll() and use a polyfill there

Enforce this method is used with a lint rule that prohibits using replaceAll() and prefers using Str.replaceAll()

Sounds good to me!

luacmartins · 2022-05-25T15:41:36Z

Just stopping by to say that I saw the same error (replaceAll is not a function) on mWeb Chrome.

Make emojis larger when multiple are sent at the same time

7b8b7d9

eVoloshchak requested a review from a team as a code owner May 6, 2022 17:18

melvin-bot bot requested review from Julesssss and Santhosh-Sellavel and removed request for a team May 6, 2022 17:18

eVoloshchak mentioned this pull request May 9, 2022

[HOLD for payment 2022-05-23][$250] Make emojis larger when multiple are sent at the same time. #8867

Closed

Julesssss assigned eVoloshchak May 9, 2022

Julesssss reviewed May 9, 2022

View reviewed changes

src/libs/EmojiUtils.js Outdated Show resolved Hide resolved

eVoloshchak added 2 commits May 9, 2022 20:29

Update comment

10fc82e

Memoize getEmojiUnicode function

316d030

Julesssss reviewed May 9, 2022

View reviewed changes

src/libs/EmojiUtils.js Outdated Show resolved Hide resolved

Return early in containsOnlyEmojis function

f89d249

Santhosh-Sellavel suggested changes May 10, 2022

View reviewed changes

src/styles/styles.js Outdated Show resolved Hide resolved

Update styles name

d3ac462

Julesssss previously approved these changes May 10, 2022

View reviewed changes

Santhosh-Sellavel approved these changes May 10, 2022

View reviewed changes

Santhosh-Sellavel suggested changes May 10, 2022

View reviewed changes

eVoloshchak added 2 commits May 10, 2022 20:17

Rename onlyEmojis style variables

e9945b3

Ignore spaces in containsOnlyEmojis function

4a78266

eVoloshchak dismissed Julesssss’s stale review via 4a78266 May 10, 2022 17:54

eVoloshchak added 2 commits May 11, 2022 17:05

Merge branch 'main' into eVoloshchak_largeEmojis

cdef131

Fix linter error

7933540

tgolen previously approved these changes May 12, 2022

View reviewed changes

Julesssss previously approved these changes May 12, 2022

View reviewed changes

Santhosh-Sellavel suggested changes May 12, 2022

View reviewed changes

Trim message only once

9c86b32

eVoloshchak dismissed stale reviews from Julesssss and tgolen via 9c86b32 May 12, 2022 17:50

Santhosh-Sellavel approved these changes May 12, 2022

View reviewed changes

Julesssss approved these changes May 13, 2022

View reviewed changes

tgolen merged commit 5c5f79b into Expensify:main May 16, 2022

OSBotify mentioned this pull request May 16, 2022

Deploy Checklist: New Expensify 2022-05-16 #9037

Closed

15 tasks

This was referenced May 18, 2022

[med] Chrome + Jaws: Success message after uploading image is not announced. #8624

Closed

[High]: Chrome+Jaws : Screen reader: Role and label are not defined for "Start a call", "Pin", "Actions", "Emoji" and "Send" controls. #8651

Closed

eVoloshchak mentioned this pull request May 18, 2022

Fix issue with app crashing in Safari on iOS13 #9063

Closed

87 tasks

This was referenced May 18, 2022

[No QA] Add polyfill for replaceAll() to fix Safari iOS 13 Expensify/expensify-common#447

Merged

Add prefer Str rule Expensify/eslint-config-expensify#51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make emojis larger when multiple are sent at the same time #8896

Make emojis larger when multiple are sent at the same time #8896

eVoloshchak commented May 6, 2022 •

edited

Loading

eVoloshchak commented May 9, 2022

Santhosh-Sellavel commented May 10, 2022

Santhosh-Sellavel commented May 10, 2022 •

edited

Loading

Santhosh-Sellavel commented May 10, 2022

Julesssss commented May 10, 2022

Julesssss left a comment

Julesssss commented May 10, 2022

Santhosh-Sellavel commented May 10, 2022

Santhosh-Sellavel left a comment

Santhosh-Sellavel May 10, 2022

Julesssss commented May 10, 2022

Santhosh-Sellavel left a comment

eVoloshchak commented May 10, 2022 •

edited

Loading

tgolen commented May 10, 2022

tgolen left a comment

tgolen commented May 12, 2022

Santhosh-Sellavel commented May 12, 2022

Julesssss left a comment

Santhosh-Sellavel left a comment

Santhosh-Sellavel May 12, 2022

eVoloshchak May 12, 2022

Santhosh-Sellavel left a comment

Julesssss left a comment

Santhosh-Sellavel commented May 15, 2022

OSBotify commented May 16, 2022

OSBotify commented May 16, 2022

OSBotify commented May 17, 2022

marcaaron commented May 17, 2022

marcaaron commented May 17, 2022

eVoloshchak commented May 18, 2022

Santhosh-Sellavel commented May 18, 2022 •

edited

Loading

marcaaron commented May 18, 2022

Santhosh-Sellavel commented May 18, 2022

luacmartins commented May 25, 2022

		fontSize: variables.fontSizeSingleEmoji,
		lineHeight: variables.fontSizeSingleEmojiHeight,

Make emojis larger when multiple are sent at the same time #8896

Make emojis larger when multiple are sent at the same time #8896

Conversation

eVoloshchak commented May 6, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

eVoloshchak commented May 9, 2022

Santhosh-Sellavel commented May 10, 2022

Santhosh-Sellavel commented May 10, 2022 • edited Loading

PR Reviewer Checklist

Santhosh-Sellavel commented May 10, 2022

Julesssss commented May 10, 2022

Julesssss left a comment

Choose a reason for hiding this comment

Julesssss commented May 10, 2022

Santhosh-Sellavel commented May 10, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel May 10, 2022

Choose a reason for hiding this comment

Julesssss commented May 10, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

eVoloshchak commented May 10, 2022 • edited Loading

tgolen commented May 10, 2022

tgolen left a comment

Choose a reason for hiding this comment

tgolen commented May 12, 2022

Santhosh-Sellavel commented May 12, 2022

Julesssss left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel May 12, 2022

Choose a reason for hiding this comment

eVoloshchak May 12, 2022

Choose a reason for hiding this comment

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented May 15, 2022

OSBotify commented May 16, 2022

OSBotify commented May 16, 2022

OSBotify commented May 17, 2022

marcaaron commented May 17, 2022

marcaaron commented May 17, 2022

eVoloshchak commented May 18, 2022

Santhosh-Sellavel commented May 18, 2022 • edited Loading

marcaaron commented May 18, 2022

Santhosh-Sellavel commented May 18, 2022

luacmartins commented May 25, 2022

eVoloshchak commented May 6, 2022 •

edited

Loading

Santhosh-Sellavel commented May 10, 2022 •

edited

Loading

eVoloshchak commented May 10, 2022 •

edited

Loading

Santhosh-Sellavel commented May 18, 2022 •

edited

Loading