-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Extract emojis from paste text #23153
Extract emojis from paste text #23153
Conversation
@parasharrajat 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also update ReportActionItemMessageEdit?
Also i am glad @parasharrajat that you have been great at pointing out each steps. 🤗 |
Done with the changes @parasharrajat. Please ping if there are any more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update ReportActionItemMessageEdit
as well.
There is a request above if you missed it. |
Please update ReportActionItemMessageEdit as well. This is the third time I am repeating this. Please read all the comments. |
@parasharrajat Please check slack https://expensify.slack.com/archives/C01GTK53T8Q/p1689935412377979 |
My bad. Sorry for the confusion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screenshots
🔲 iOS / native
🔲 iOS / Safari
🔲 MacOS / Desktop
🔲 MacOS / Chrome
🔲 Android / Chrome
🔲 Android / native
Do i need to send them individually ? As i have already attached them in PR. |
Screenshots/VideosWebEmojiFrequentPaste-Chrome-min.mp4Mobile Web - ChromeEmojiFrequentPaste-WebChrome-min.mp4Mobile Web - SafariEmojiFrequentPaste-WebSafari-min.mp4DesktopEmojiFrequentPaste-Desktop-min.mp4iOSEmojiFrequentPaste-iOS-min.mp4AndroidEmojiFrequentPaste-Android-min.mp4 |
Lol, those screenshots' comment is for me. There is no action left for you for now. |
Will send again its gonna take some time 😢 . |
It does not work for me on the web. Screen.Recording.2023-07-21.at.9.20.49.PM.mov |
Solution 1function extractEmojis(text) {
const emojis = [];
if (!text) {
return [];
}
// Parsed Emojis with skin tone - Eg: ['👩🏻', '👩🏼', '👩🏽', '👩🏾', '👩🏿', '👩🏻', '👩🏼', '👩🏽', '👩🏾', '👩🏿']
const parsedEmojis = text.match(CONST.REGEX.EMOJIS);
if (!parsedEmojis) {
return [];
}
const alreadyParsedEmojis = new Set();
for (let i = 0; i < parsedEmojis.length; i++) {
const character = parsedEmojis[i];
const emoji = Emojis.emojiCodeTableWithSkinTones[character];
if (emoji && !alreadyParsedEmojis.has(emoji.code)) {
alreadyParsedEmojis.add(emoji.code);
emojis.push(emoji);
}
}
return emojis;
} Solution 2function extractEmojis(text) {
if (!text) {
return [];
}
// Parsed Emojis with skin tone - Eg: ['👩🏻', '👩🏼', '👩🏽', '👩🏾', '👩🏿', '👩🏻', '👩🏼', '👩🏽', '👩🏾', '👩🏿']
const parsedEmojis = text.match(CONST.REGEX.EMOJIS);
if (!parsedEmojis) {
return [];
}
const emojiCodes = new Set();
return parsedEmojis.reduce((acc, character) => {
const emoji = Emojis.emojiCodeTableWithSkinTones[character];
if (emoji && !emojiCodes.has(emoji.code)) {
emojiCodes.add(emoji.code);
acc.push(emoji);
}
return acc;
}, []);
} |
I didn't understand at all the purpose of posting these diffs. Are you asking to pick one of these? Can you please tell me your reasoning for the suggestions I posted? Again, if you think those suggestions work. I see that that solution can eliminate pushing into both Set and array. let's just do this and later Marc can suggest improvements if any. |
Going with solution one. |
|
Done with the changes @parasharrajat |
But @parasharrajat don't we need the whole object instead of just emoji ? |
Good question. Let me check. |
|
src/libs/EmojiUtils.js
Outdated
} | ||
|
||
const emojis = []; | ||
const alreadyParsedEmojis = new Set(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const alreadyParsedEmojis = new Set(); | |
// Text can contain similar emojis as well as their skin tone variants. Create a Set to remove duplicate emojis from the search. | |
const alreadyParsedEmojis = new Set(); |
src/libs/EmojiUtils.js
Outdated
for (let i = 0; i < parsedEmojis.length; i++) { | ||
const character = parsedEmojis[i]; | ||
const emoji = Emojis.emojiCodeTableWithSkinTones[character]; | ||
if (emoji && !alreadyParsedEmojis.has(emoji.code)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (emoji && !alreadyParsedEmojis.has(emoji.code)) { | |
// Add the parsed emoji to the final emojis if not already present. | |
if (emoji && !alreadyParsedEmojis.has(emoji.code)) { |
src/libs/EmojiUtils.js
Outdated
} | ||
|
||
const emojis = []; | ||
const alreadyParsedEmojis = new Set(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const alreadyParsedEmojis = new Set(); | |
const foundEmojiCodes = new Set(); |
Rename the variable to better match its content.
let's do the preceding changes instead. I am not against using reduce but this primitive logic is fine too IMO. |
Done @parasharrajat |
Finally Done ✅ |
LGTM. Let's wait for @marcaaron's review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the teamwork! Looks much better 👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.57-0 🚀
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.58-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.57-6 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.58-5 🚀
|
Details
Fixed Issues
$ #21786
PROPOSAL: #21786 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
EmojiPasteFix-Chrome-min.mp4
Mobile Web - Chrome
EmojiPasteFix-Mobile-Chrome-min.mp4
Mobile Web - Safari
EmojiPasteFix-Mobile-Safari-min.mp4
Desktop
EmojiPasteFix-Desktop-min.mp4
iOS
EmojiPasteFix-iOS-min.mp4
Android
EmojiPasteFix-Android-min.mp4