Conversation
melvin-bot
bot
requested review from
arosiclair
and removed request for
a team
|
@arosiclair @fedirjh I created another PR in expensify-common here since the previous PR is reverted. Ref: Expensify/App#36437 |
| { | ||
| name: 'emoji', | ||
| regex: CONST.REG_EXP.EMOJIS, | ||
| replacement: match => `<emoji>${match}</emoji>` | ||
| }, | ||
|
|
There was a problem hiding this comment.
Let's add a description to this rule?
lib/CONST.jsx
Outdated
| * @type RegExp | ||
| */ | ||
| EMOJIS: /[\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu, | ||
| EMOJI: /[\p{Extended_Pictographic}\u200d\u{1f1e6}-\u{1f1ff}\u{1f3fb}-\u{1f3ff}\u{e0020}-\u{e007f}\u20E3\uFE0F]|[#*0-9]\uFE0F?\u20E3/gu, |
There was a problem hiding this comment.
Is this change safe? This maybe used in other repositories as well. Let's keep EMOJIS as it's been and implement new separate regexp for our use case.
lib/CONST.jsx
Outdated
| * | ||
| * @type RegExp | ||
| */ | ||
| EMOJIS: /[\p{Extended_Pictographic}](\u200D[\p{Extended_Pictographic}]|[\u{1F3FB}-\u{1F3FF}]|[\u{E0020}-\u{E007F}]|\uFE0F|\u20E3)*|[\u{1F1E6}-\u{1F1FF}]{2}|[#*0-9]\uFE0F?\u20E3/gu, |
There was a problem hiding this comment.
Why do we implement 2 Regex for emojis?
There was a problem hiding this comment.
Expensify/App#35838 (comment) Coming from this comment this regex can cover this case by only applying one emoji tag for the emoji that is made up by some different emojis.
|
@dukenv0307 Friendly bump. |
| + '[<emoji>😄</emoji>] <a href="mailto:abc@gmail.com">abc@gmail.com</a> ' | ||
| + '[<emoji>😄</emoji>]((<a href="mailto:abc@gmail.com">abc@gmail.com</a>)) ' | ||
| + '[<emoji>😄</emoji><a href="mailto:abc@gmail.com">abc@gmail.com</a>](<a href="mailto:abc@gmail.com">abc@gmail.com</a>) ' | ||
| + '[<emoji>😄</emoji> <a href="mailto:abc@gmail.com">abc@gmail.com</a> ](<a href="mailto:abc@gmail.com">abc@gmail.com</a>) ' |
There was a problem hiding this comment.
Let's add a test for Composite emojis.
arosiclair
left a comment
arosiclair
left a comment
There was a problem hiding this comment.
LGTM all yours @stitesExpensify
|
@dukenv0307 @stitesExpensify @arosiclair Just an FYI this was reverted because of Expensify/App#38169 We should have probably not merged this until Expensify/App#37814 was ready to go but understandably these things are hard to forsee. |
It should have been reverted alongside with: |
|
Ahhhh... Well now you guys have no excuse then 😂 |
5 participants
Fixed Issues
$ Expensify/App#34307
Tests
htmlof the message and verify that the emoji is wrapped in aemojitagQA
htmlof the message and verify that the emoji is wrapped in aemojitagOffline test
Same as above
Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop