Handle emoji tooltip #35838
Handle emoji tooltip #35838
Conversation
|
@fedirjh My Android and IOS simulator don't work now. Please help me to test on these platforms. |
There was a problem hiding this comment.
Please help me to test on these platforms.
@dukenv0307 Sure.
| !_.isUndefined(props.draftMessage) | ||
| ? null | ||
| : props.action.pendingAction || | ||
| (props.action.isOptimisticAction ? CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD : '') || | ||
| lodashGet(props, 'iouReport.pendingFields.preview') |
There was a problem hiding this comment.
Missed restoring some changes to test locally.
|
We can completely test now. |
|
@dukenv0307 Could you please merge main ? Thank you |
|
@fedirjh I updated. |
|
@dukenv0307 Was the
This is already live as per this comment https://expensify.slack.com/archives/C01GTK53T8Q/p1707153064290739?thread_ts=1706851954.228519&cid=C01GTK53T8Q |
When I tested it locally, it was not. I need to hard code in |
Can you please hard code with this to test locally? |
Cool, I didn't realize this when creating the PR. |
|
We have to bump |
|
@fedirjh Bumped the expensify-common version. |
|
@fedirjh Thanks for your great solution, I updated this and tested. |
| const htmlContent = styleAsDeleted ? `<del>${html}</del>` : html; | ||
| const htmlWithDeletedTag = styleAsDeleted ? `<del>${html}</del>` : html; | ||
|
|
||
| const containsOnlyEmojis = EmojiUtils.containsOnlyEmojis(text); |
There was a problem hiding this comment.
@dukenv0307 I got a little bug with offline feedback (deleted action); when a comment that only contains emojis is deleted in offline mode, the style is not applied correctly :
| current | expected |
|---|---|
 |
 |
I think the simplest solution in this case is to use the text source of the comment when it's pending deletion, it will fix the style issue, but it will not render the tooltip. I think that's fine since this comment will be deleted anyway. cc @stitesExpensify what do you think?
There was a problem hiding this comment.
@dukenv0307 Have you got a chance to look at this one? I think this is a blocker for this PR.
There was a problem hiding this comment.
The changes that should fix this one are suggested here #35838 (comment)
There was a problem hiding this comment.
I saw that and I'm waiting @stitesExpensify for confirming this.
There was a problem hiding this comment.
I think that we can ignore this for now. I am creating a new issue to prevent markdown from affecting emojis as a whole, so this will be fixed in the future in a different way
There was a problem hiding this comment.
@fedirjh Updated this case.
| const htmlContent = styleAsDeleted ? `<del>${html}</del>` : html; | ||
| const htmlWithDeletedTag = styleAsDeleted ? `<del>${html}</del>` : html; | ||
|
|
||
| const containsOnlyEmojis = EmojiUtils.containsOnlyEmojis(text); |
There was a problem hiding this comment.
We should move this to the top to avoid code duplication
| @@ -52,7 +52,10 @@ function TextCommentFragment({fragment, styleAsDeleted, source, style, displayAs | |||
| // Only render HTML if we have html in the fragment | |||
| if (!differByLineBreaksOnly) { | |||
There was a problem hiding this comment.
| if (!differByLineBreaksOnly) { | |
| if (!differByLineBreaksOnly && !(containsOnlyEmojis && styleAsDeleted)) { |
This change should fix the offline style , we just need to move containsOnlyEmojis to the top.
|
Bug: Large Emoji is cut off at the top when the comment is edited CleanShot.2024-02-26.at.19.20.44.mp4 |
|
@dukenv0307 To address the last bug, I think we can explore creating a separate version of |
There was a problem hiding this comment.
Create a new directory shouldRenderAsText and add index.native.ts, index.ts and types.ts
| const htmlContent = styleAsDeleted ? `<del>${html}</del>` : html; | ||
| const htmlWithDeletedTag = styleAsDeleted ? `<del>${html}</del>` : html; | ||
|
|
||
| const containsOnlyEmojis = EmojiUtils.containsOnlyEmojis(text); |
There was a problem hiding this comment.
@dukenv0307 Have you got a chance to look at this one? I think this is a blocker for this PR.
|
Both bugs were fixed 🚀
|
|
On web: bug was fixed also 🚀 CleanShot.2024-02-28.at.07.29.12.mp4 |
There was a problem hiding this comment.
Looks good to me and tests well, over to you @stitesExpensify
|
Changes look good but there are conflicts :( Just a quick package change and we should be good to go! |
|
@stitesExpensify I merged main and resolved the conflict. |
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.4.47-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.47-10 🚀
|
Details
Display a tooltip when hovering over emoji in report action
Fixed Issues
$ #34307
PROPOSAL: #34307 (comment)
Tests
Offline tests
Same as above
QA Steps
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.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
Screen.Recording.2024-02-05.at.23.41.52.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2024-02-05.at.23.40.44.mov
MacOS: Chrome / Safari
Screen.Recording.2024-02-05.at.23.38.47.mov
MacOS: Desktop
Screen.Recording.2024-02-05.at.23.46.52.mov