-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Handle emoji tooltip and fix regression #37814
Conversation
if (styleAsMuted) { | ||
htmlWithTag = `<muted-text>${htmlWithTag}<muted-text>`; | ||
} |
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.
Let's keep this block.
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.
Updated this. I'm updating to fix the case emoji inside codeblock, will open the PR after I fix this case.
package-lock.json
Outdated
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.
Why this change is needed?
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.
This is my mistake. Reverted this.
|
||
// Create a temporary solution to display when there are emojis in the inline code block | ||
// We can remove this after https://github.com/Expensify/App/issues/14676 is fixed | ||
export default function removeEmojiTag(defaultRendererProps: TDefaultRendererProps<TTextOrTPhrasing>): string { |
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.
How does this remove the emoji tag?
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.
@fedirjh If data
field of tnode
is undefined
, it means we have emoji tag inside inline codeblock and the text is divided into children data. So we can get the current text by merging the children data, this is correct because other tags will not appear inside inline codeblock except emoji tag.
I think we can rename this function to getCurrentData
or getCurrentText
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.
Yes, it makes sense to rename it.
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.
I updated the name of function.
Reviewer Checklist
Screenshots/VideosMacOS: DesktopCleanShot.2024-03-15.at.17.41.32.mp4 |
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.
@dukenv0307 let's bump the expensify-common as well in this PR.
|
||
// Create a temporary solution to display when there are emojis in the inline code block | ||
// We can remove this after https://github.com/Expensify/App/issues/14676 is fixed | ||
export default function removeEmojiTag(defaultRendererProps: TDefaultRendererProps<TTextOrTPhrasing>): string { |
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.
Yes, it makes sense to rename it.
@fedirjh Expensify/expensify-common#664, The previous PR is reverted so I created another one here. |
@fedirjh As suggestion here Expensify/expensify-common#664 (comment), I update the version of expensify-common to the commit hash of the PR above for testing. |
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.
Looks good and tests well.
Are there negative consequences of merging the expensify-common lib and not merging this? Just wondering how close our timing needs to be |
@dukenv0307 If you are around now I can merge the common PR and then you can update and I will merge immediately. Ping me on slack if you have time. |
If the expensify-common lib PR gets merged and bumped in the App without this PR, then it will cause some regressions until this PR is merged. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Just to note that this PR skipped testing other changes that the version bump of expensify-common involves. |
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.4.56-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.56-8 🚀
|
It seems that the issue #49053 may have been overlooked in this PR. |
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 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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.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
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