[TS migration] Migrate 'EmojiSuggestions.js' component to TypeScript#30416
Conversation
| if (!item.name) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why was this condition removed?
| } | ||
| }); | ||
| emojis | ||
| .filter((item: Emoji): item is SimpleEmoji => !(item as HeaderEmoji).header) |
There was a problem hiding this comment.
Could you explain the purpose of this filter()?
There was a problem hiding this comment.
@fabioh8010 removing of the if condition and adding of this filter are related to each other.
emojis is an array that has one header emoji
{
header: true,
icon: Smiley,
code: 'smileysAndEmotion',
}
and other usual emojis.
Header emoji is not used in emoji suggestions and was filtered out before with a check.
if (!item.name) {
return;
}
I replaced it with a filter instead to add type narrowing to identify that only simple emojis are used in the suggestions.
Btw, before the migration of the EmojiTrie file it also checked for the header field instead of name to filter header emoji out.
There was a problem hiding this comment.
LGTM, maybe let's wait for EmojiUtils PR before proceeding with this PR?
|
@blazejkustra okay, I'll monitor it |
# Conflicts: # src/components/EmojiSuggestions.tsx # src/components/Text.tsx
melvin-bot
Bot
requested review from
eVoloshchak
and removed request for
a team
|
@eVoloshchak 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] |
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
| {styledTextArray.map(({text, isColored}, i) => ( | ||
| <Text | ||
| /* eslint-disable-next-line react/no-array-index-key */ | ||
| key={`${text}+${i}`} |
There was a problem hiding this comment.
This isn't related to this migration and was present before, but still would be nice to fix
We shouldn't use array index in keys, elements should have stable keys when array is filtered/reordered
| key={`${text}+${i}`} | |
| key={text} |
There was a problem hiding this comment.
@eVoloshchak The only concern I have is that potentially, in a rare case, there can be two the same texts.
Maybe will be better to use isColored as well
key={`${text}+${isColored}`}
There was a problem hiding this comment.
The only concern I have is that potentially, in a rare case, there can be two the same texts
There shouldn't be two identical emojis, but it's better to account for such cases, you're right. I like the isColored idea
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25045 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
✋ 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/johnmlee101 in version: 1.3.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.99-0 🚀
|
|
🚀 Deployed to staging by https://github.com/johnmlee101 in version: 1.4.0-0 🚀
|
6 participants
Details
[TS migration] Migrate 'EmojiSuggestions.js' component to TypeScript
Fixed Issues
$ #25045
PROPOSAL: N/A
Tests
:and the name of the emoji, for example,:love.Offline tests
N/A
QA Steps
:and the name of the emoji, for example,:love.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop