Render Emoji when typed with emoji code - #10165
Conversation
|
I also think unit tests would be really valuable here. |
Ya makes sense! |
roryabraham
left a comment
There was a problem hiding this comment.
Also, I think it would be useful to implement a method called getAllMatchingWords(substr) that:
- Finds the node matching the substring
- Finds all leaf nodes that are children of that substring and returns them as an array.
So for example, given a Trie containing the words cape and cats, we would expect to see a Trie like this:
c
|
a
/ \
p t
/ \
e s
So if we ran getAllMatchingWords('ca') we would get ['cape', 'cats'] back.
|
@Karim-30 Any update? |
|
@Santhosh-Sellaveltomorrow inshallah. |
4f790a8
|
@Santhosh-Sellavel I think you're the assigned C+ for this PR, so let's prioritize getting it reviewed and tested soon. It has been open a long time so would be great to get it over the finish line |
There was a problem hiding this comment.
tests well! cc @Santhosh-Sellavel
(sidenote im not the c+ here)
|
Reviewing this now |
|
Santhosh-Sellavel
left a comment
There was a problem hiding this comment.
LGTM, tests well!
|
@Santhosh-Sellavel you need to add screenshots for all platforms as part of a change to the checklist |
|
Oh Crap ya I missed that, will do that later today, AFK now. |
AndroidScreen.Recording.2022-10-21.at.12.36.42.AM.moviOS & mWebSimulator.Screen.Recording.-.iPhone.13.-.2022-10-21.at.00.33.44.mp4Desktop & WebScreen.Recording.2022-10-21.at.12.31.14.AM.mov |
|
Woohoo! Let's ship it! |
|
@stitesExpensify looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
|
Tests had passed. :that: is a bug |
|
✋ 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 @stitesExpensify in version: 1.2.19-0 🚀
|
|
🚀 Deployed to production by @chiragsalian in version: 1.2.19-2 🚀
|
|
Heads up! This PR caused one issue #14876. We forgot to test skin tone for emojis on this. |
|
@parasharrajat We built |
|
Can you share a link to the comment where it was mentioned out of scope? |
Details
this PR makes it possible to see emojis when typing the emoji code inside ::, for example, if you type
Hi :smile:in the Composer you should see Hi 😄 notHi :smile:.Fixed Issues
$ #4324
$ #4324 (comment)
Tests
Hi :smile:.Hi 😄notHi :smile:.PR Review Checklist
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)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)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)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)QA Steps
Hi :smile:.Hi 😄notHi :smile:.Screenshots
Web
web.mov
Mobile Web - Chrome
mweb.chrome.mov
Mobile Web - Safari
mweb.mov
Desktop
Desktop.mov
iOS
IOS.mov
Android
android.mov