feat: support Spanish emojis by dummy-1111 · Pull Request #20828 · Expensify/App

dummy-1111 · 2023-06-15T14:00:32Z

Details

Fixed Issues

$ #16086
PROPOSAL: #16086 (comment)

Tests

Open NewDot and go to any chat
Hovering any message shows reaction bar. Hover any emoji on the reaction bar and verify that emoji shortcode is translated
Hover any emoji reactions under the message and verify that the tooltip shows translated emoji short code. On mobile long press any emoji reactions and verify
Clicking the emoji+ button shows the emoji picker popup.
In this popup, verify that
- search by Spanish keyword works
- hovering category icons shows translated category names
- skintone selection works for emojis supporting skintone
- selecting emoji toggles emoji reaction
In the composer, verify that emoji suggestion and replacing works
Clicking the emoji button in the composer shows the emoji picker popup. Verify all items in step 5

Verify that no errors appear in the JS console

Offline tests

Open NewDot and go to any chat
Hovering any message shows reaction bar. Hover any emoji on the reaction bar and verify that emoji shortcode is translated
Hover any emoji reactions under the message and verify that the tooltip shows translated emoji short code. On mobile long press any emoji reactions and verify
Clicking the emoji+ button shows the emoji picker popup.
In this popup, verify that
- search by Spanish keyword works
- hovering category icons shows translated category names
- skintone selection works for emojis supporting skintone
- selecting emoji toggles emoji reaction
In the composer, verify that emoji suggestion and replacing works
Clicking the emoji button in the composer shows the emoji picker popup. Verify all items in step 5

QA Steps

Open NewDot and go to any chat
Hovering any message shows reaction bar. Hover any emoji on the reaction bar and verify that tooltip is translated in preferred language
Hover any emoji reactions under the message and verify that the tooltip shows translated emoji short code. On mobile long press any emoji reactions and verify
Clicking the emoji+ button shows the emoji picker popup.
In this popup, verify that
- search by Spanish keyword works when preferred locale is English
- search by Spanish and English keyword works when preferred locale is Spanish
- hovering category icons shows translated category names
- skintone selection works for emojis supporting skintone
- selecting emoji toggles emoji reaction
In the composer, verify that emoji suggestion and replacing works
Clicking the emoji button in the composer shows the emoji picker popup. Verify all items in step 5
Run all reaction and emoji picker related regressions

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

16086_mac_chrome.mp4

Mobile Web - Chrome

16086_anrdoid_chrome.mp4

Mobile Web - Safari

16086_ios_safari.mp4

Desktop

16086_mac_desktop.mp4

iOS

16086_ios_native.mp4

Android

16086_anroid_native.mp4

melvin-bot · 2023-06-16T00:50:52Z

@0xmiroslav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

0xmiroslav · 2023-06-16T17:02:13Z

    QUICK_REACTIONS: [
        {
-            name: '+1',
+            shortcode: {en: '+1', es: '+1'},


Can you stick to original structure? I don't see any reason why name should be changed to shortcode

This is for clarity. They are not names of emojis, just short codes of them. I mentioned this in issue before

Strictly, they are "short name", not code.
If I follow you, code = '😄', short code = 'smile',
Why code length should be smaller than short code length?
As we don't support name for now, we can just use name instead of short name.
So let's keep original structure.

I think that the name should always be in english. In the database we save the reactions like this:

{"emoji":"heart","users":[{"accountID":8505565,"skinTone":-1}...]

If we start changing that then you would be able to react with the same emoji in both english and spanish.

0xmiroslav · 2023-06-16T17:02:43Z

+            shortcode: {en: '+1', es: '+1'},
            code: '👍',
-            types: ['👍🏿', '👍🏾', '👍🏽', '👍🏼', '👍🏻'],
+            types: ['👍🏻', '👍🏼', '👍🏽', '👍🏾', '👍🏿'],


NAB but why orders changed?

In ND app, their order is the same as is. That's the reason I reversed the order. I think this is reasonable.

I am also confused. As far as I can tell the types are already displayed in that order, so I don't understand why we need to change this

As you can see below, skintone numbering is weird. I think this change is reasonable.

App/assets/emojis.js

Lines 56 to 81 in 83ab016

const skinTones = [

{

code: '🖐',

skinTone: -1,

},

{

code: '🖐🏻',

skinTone: 4,

},

{

code: '🖐🏼',

skinTone: 3,

},

{

code: '🖐🏽',

skinTone: 2,

},

{

code: '🖐🏾',

skinTone: 1,

},

{

code: '🖐🏿',

skinTone: 0,

},

];

Please make sure that this order change doesn't cause any regressions even if it's minor.

Yeah. I'm sure

0xmiroslav · 2023-06-16T17:03:42Z

    headerEmojis: PropTypes.arrayOf(
        PropTypes.shape({
-            code: PropTypes.string.isRequired,
+            name: PropTypes.string.isRequired,


Same here. I don't see any reason of code -> name

Headers has no code unlike emojis. It has only name and icon. I wanted to add meanings to the properties

We already have localized strings here:

App/src/languages/en.js

Lines 1221 to 1233 in edf529e

headers: {

frequentlyUsed: 'Frequently Used',

smileysAndEmotion: 'Smileys & Emotion',

peopleAndBody: 'People & Body',

animalsAndNature: 'Animals & Nature',

foodAndDrink: 'Food & Drinks',

travelAndPlaces: 'Travel & Places',

activities: 'Activities',

objects: 'Objects',

symbols: 'Symbols',

flags: 'Flags',

},

},

We can still use frequentlyUsed, etc for code.

The current Spanish copies are already verified by marketing team and cannot be changed.

Before:

After:

Please revert back to original structure where header code is smileysAndEmotion and copies are localized in en.js and es.js

I'll change the headers back to original Spanish.
But let's keep the current emoji data structure as is. We have all Spanish emojis in its own structure, and we treat headers as emojis as well. Why don't we translate headers in the same place? I don't understand why headers should be in en/es.js. We can safely remove all emojis from en/es.js. This is more clear and reasonable, I think.

@stitesExpensify do you agree with this new structure?

I think we should think about extension of supported languages, not about original structure.

I'm confused @s-alves10 what is the benefit of this change? In the future we're going to add other files like de.js which with have german translations etc. for the whole app, so it seems like keeping the current structure makes more sense than changing it

0xmiroslav

Your updated structure of name -> shortcode, header code -> name can be follow-up if needed.
In this PR, let's reduce code diff as much as possible and try to keep original structure but just add localization.

0xmiroslav · 2023-06-16T17:34:15Z

    QUICK_REACTIONS: [
        {
-            name: '+1',
+            shortcode: {en: '+1', es: '+1'},


Strictly, they are "short name", not code.
If I follow you, code = '😄', short code = 'smile',
Why code length should be smaller than short code length?
As we don't support name for now, we can just use name instead of short name.
So let's keep original structure.

0xmiroslav · 2023-06-16T17:35:58Z

    headerEmojis: PropTypes.arrayOf(
        PropTypes.shape({
-            code: PropTypes.string.isRequired,
+            name: PropTypes.string.isRequired,


We already have localized strings here:

App/src/languages/en.js

Lines 1221 to 1233 in edf529e

headers: {

frequentlyUsed: 'Frequently Used',

smileysAndEmotion: 'Smileys & Emotion',

peopleAndBody: 'People & Body',

animalsAndNature: 'Animals & Nature',

foodAndDrink: 'Food & Drinks',

travelAndPlaces: 'Travel & Places',

activities: 'Activities',

objects: 'Objects',

symbols: 'Symbols',

flags: 'Flags',

},

},

We can still use frequentlyUsed, etc for code.

stitesExpensify

I'm a bit confused about some of these changes. I was under the impression that we were just adding spanish keywords to the file, and creating a spanish Trie. If you could clarify why other changes were necessary that would be great @s-alves10

stitesExpensify · 2023-06-16T18:57:10Z

+            shortcode: {en: '+1', es: '+1'},
            code: '👍',
-            types: ['👍🏿', '👍🏾', '👍🏽', '👍🏼', '👍🏻'],
+            types: ['👍🏻', '👍🏼', '👍🏽', '👍🏾', '👍🏿'],


I am also confused. As far as I can tell the types are already displayed in that order, so I don't understand why we need to change this

stitesExpensify · 2023-06-16T18:59:29Z

    QUICK_REACTIONS: [
        {
-            name: '+1',
+            shortcode: {en: '+1', es: '+1'},


I think that the name should always be in english. In the database we save the reactions like this:

{"emoji":"heart","users":[{"accountID":8505565,"skinTone":-1}...]

If we start changing that then you would be able to react with the same emoji in both english and spanish.

stitesExpensify · 2023-06-16T19:02:05Z

    headerEmojis: PropTypes.arrayOf(
        PropTypes.shape({
-            code: PropTypes.string.isRequired,
+            name: PropTypes.string.isRequired,


I'm confused @s-alves10 what is the benefit of this change? In the future we're going to add other files like de.js which with have german translations etc. for the whole app, so it seems like keeping the current structure makes more sense than changing it

dummy-1111 · 2023-06-16T19:24:50Z

@stitesExpensify @0xmiroslav

Thank you for your feedback
Let me explain.

Emojis have its own data structure supporting multiple languages.

In order to storing all translations regarding emojis with the same structure, I changed quick emojis as well. Those have the same structure as normal emojis.
The order of skintones is different from what we see in the ND. It makes me confused and would make other developers as well. That's why I changed its order in the code
Supporting more languages isn't a problem in our current structure. All emoji translations would be removed under languages folder and we can care only other translations. The resource I used supports many other languages. Separating emojis and others in translation isn't more convenient?
Changing emoji data structure leads to a little more changes in the project but those are required in order to keep our emojis data consistently over the entire project.
When interacting backend, we use its English name as before

…ategory Spanish

0xmiroslav · 2023-06-16T21:39:54Z

Your updated structure of name -> shortcode, header code -> name can be follow-up if needed. In this PR, let's reduce code diff as much as possible and try to keep original structure but just add localization.

I am not opposed your suggestion but this was what I suggested.

@stitesExpensify what do you suggest?

stitesExpensify · 2023-06-16T21:49:53Z

I agree. I don't think that we need to make such big changes in this PR. All we really want from this PR is for typing :sonrisa: to come up with 😄 and everything else to stay the same

0xmiroslav · 2023-07-03T06:38:47Z

@s-alves10 lint failing. Please make sure to test all possible cases. Though I already tested, there might still be any missing case.

dummy-1111 · 2023-07-04T19:38:15Z

@stitesExpensify

Will you take a look at the PR again?

0xmiroslav · 2023-07-04T19:39:20Z

It's US holiday today. Let's wait one more day

stitesExpensify · 2023-07-05T15:16:55Z

Hi there! Taking a look!

0xmiroslav · 2023-07-05T15:21:20Z

@s-alves10 please fix conflict

stitesExpensify

Code is looking great! Just a couple of small questions before I approve

dummy-1111 · 2023-07-05T17:26:26Z

@stitesExpensify

Please take a look at the changes. Thank you

OSBotify · 2023-07-07T22:22:35Z

✋ 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 · 2023-07-11T00:20:37Z

🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.39-0 🚀

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

OSBotify · 2023-07-13T01:44:39Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.39-11 🚀

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

jjcoffee · 2023-09-14T15:44:17Z

Just coming from this issue that fixed a bug that this PR introduced that caused duplicate suggestions to appear. Since we add the English Trie node with the suggestion's name in English, this won't match the equivalent Spanish emoji name, so it gets added as a separate match here:

App/src/libs/EmojiUtils.js

Line 381 in 3f9f1e6

    
           if (nodes[j].metaData.code && !_.find(matching, (obj) => obj.name === nodes[j].name)) {

sangar-1028 added 11 commits June 15, 2023 03:30

feat: new emoji database

d38f775

feat: update quick reactions(support es)

08496e8

feat: all emojis and categories support es

51ac473

fix: import error

07b59e3

lint error

8c18175

fix: merge main and fix conflicts

91839ed

fix: emoji suggestion bug

d9835f9

fix: Emoji and Report tests

b309e68

fix: conflicts

d3778b2

fix: nested property

23bb285

fix: prettier

99b8343

dummy-1111 marked this pull request as ready for review June 16, 2023 00:50

dummy-1111 requested a review from a team as a code owner June 16, 2023 00:50

melvin-bot Bot requested review from 0xmiroslav and removed request for a team June 16, 2023 00:50

dummy-1111 mentioned this pull request Jun 16, 2023

[HOLD for payment 2023-07-20] [$4000] Emoji reaction - Emoji reaction names are not in Spanish language #16086

Closed

6 tasks

0xmiroslav reviewed Jun 16, 2023

View reviewed changes

stitesExpensify requested changes Jun 16, 2023

View reviewed changes

fix: remove people-and-body category and corresponding icon, change c…

f742447

…ategory Spanish

sangar-1028 added 6 commits June 17, 2023 06:36

fix: header emojis back to original

2551336

fix: toggleReaction, quick emoji back to original

61be178

Merge branch 'main' into fix/issue-16086

d9820a1

fix: remove wrong comments

e9eccaf

fix: rename getEmojiName -> getLocalizedEmojiName

fe1d94b

fix: lint error

1977161

0xmiroslav suggested changes Jul 3, 2023

View reviewed changes

Comment thread src/components/Reactions/QuickEmojiReactions/BaseQuickEmojiReactions.js

sangar-1028 added 2 commits July 3, 2023 01:25

Merge branch 'main' into fix/issue-16086

f9192ee

fix: preferredLocaled added to BaseQuickEmojiReactions

d7d91b8

0xmiroslav approved these changes Jul 3, 2023

View reviewed changes

fix: lint error

bf8e80e

0xmiroslav approved these changes Jul 3, 2023

View reviewed changes

fix: conflicts

6461f53

stitesExpensify requested changes Jul 5, 2023

View reviewed changes

Comment thread src/libs/actions/Report.js Outdated

Comment thread src/libs/EmojiTrie.js

sangar-1028 added 2 commits July 5, 2023 11:32

fix: conflicts

cb4e9cf

fix: rename emojiToReact -> reactionEmoji

a3cd2c9

stitesExpensify approved these changes Jul 7, 2023

View reviewed changes

stitesExpensify merged commit cc7f0bb into Expensify:main Jul 7, 2023

OSBotify mentioned this pull request Jul 10, 2023

Deploy Checklist: New Expensify 2023-07-10 #22598

Closed

76 tasks

0xmiroslav mentioned this pull request Jul 11, 2023

Migrate ReportActionCompose to function #18648

Merged

56 tasks

lanitochka17 mentioned this pull request Jul 11, 2023

[$1000] [HOLD for payment 2023-07-21] [HOLD for payment 2023-07-20] Emoji - Frequently Used emojis display default skintone color instead of selected #22654

Closed

6 tasks

francoisl mentioned this pull request Jul 11, 2023

Add types in frequently used emojis #22662

Merged

58 tasks

lanitochka17 mentioned this pull request Jul 11, 2023

Emoji - Emoji search results in English are displayed for account with Spanish language set #22687

Closed

6 tasks

ayazhussain79 mentioned this pull request Jul 24, 2023

[HOLD for payment 2023-10-18] [$1000] Certain country flags are not searchable when searching them in Spanish language #23429

Closed

6 tasks

dummy-1111 deleted the fix/issue-16086 branch August 22, 2023 15:59

jjcoffee mentioned this pull request Aug 24, 2023

[HOLD for payment 2023-09-18] [$1000] Both English & Spanish Emoji are present while making default language spanish #25735

Closed

6 tasks

	const skinTones = [
	{
	code: '🖐',
	skinTone: -1,
	},
	{
	code: '🖐🏻',
	skinTone: 4,
	},
	{
	code: '🖐🏼',
	skinTone: 3,
	},
	{
	code: '🖐🏽',
	skinTone: 2,
	},
	{
	code: '🖐🏾',
	skinTone: 1,
	},
	{
	code: '🖐🏿',
	skinTone: 0,
	},
	];

	headers: {
	frequentlyUsed: 'Frequently Used',
	smileysAndEmotion: 'Smileys & Emotion',
	peopleAndBody: 'People & Body',
	animalsAndNature: 'Animals & Nature',
	foodAndDrink: 'Food & Drinks',
	travelAndPlaces: 'Travel & Places',
	activities: 'Activities',
	objects: 'Objects',
	symbols: 'Symbols',
	flags: 'Flags',
	},
	},

Uh oh!

Conversation

dummy-1111 commented Jun 15, 2023 • edited by stitesExpensify Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot Bot commented Jun 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dummy-1111 Jun 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xmiroslav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stitesExpensify left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dummy-1111 commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xmiroslav commented Jun 16, 2023

Uh oh!

stitesExpensify commented Jun 16, 2023

Uh oh!

Uh oh!

0xmiroslav commented Jul 3, 2023

Uh oh!

dummy-1111 commented Jul 4, 2023

Uh oh!

dummy-1111 commented Jun 15, 2023 •

edited by stitesExpensify

Loading

dummy-1111 Jun 17, 2023 •

edited

Loading

dummy-1111 commented Jun 16, 2023 •

edited

Loading