-
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
[Mentions v2] Parse <mention-report> when displaying reportAction #39906
[Mentions v2] Parse <mention-report> when displaying reportAction #39906
Conversation
src/components/HTMLEngineProvider/HTMLRenderers/MentionRoomRenderer.tsx
Outdated
Show resolved
Hide resolved
src/components/HTMLEngineProvider/HTMLRenderers/MentionRoomRenderer.tsx
Outdated
Show resolved
Hide resolved
src/components/HTMLEngineProvider/HTMLRenderers/MentionRoomRenderer.tsx
Outdated
Show resolved
Hide resolved
src/components/HTMLEngineProvider/HTMLRenderers/MentionRoomRenderer.tsx
Outdated
Show resolved
Hide resolved
Linking to another room only seems to work from rooms, not from a normal chat. Screen.Recording.2024-04-10.at.12.07.25.mov |
I think we should call the renderer |
Actually @rlinoz @puneetlath When user types by hand room mention (auto complete is disabled for chats which are not in group policy) we will parse it and send to backend (parser is not aware of chat type). Will backend filtered this tag out? |
Changed 👌🏼 |
Yes, if a mention happens outside of a specific policy we will remove the tag. |
So if we have only optimistic version of the report action the expected behaviour would be to check if mentions are allowed in current report and if not just return null and not render it at all? |
I think we should strip out the tag and render as plain text if the mention is happening in a report that is not allowed. |
Just to be sure, we are going to do that in this PR right? |
Yeah @war-in will take care of it tommorow |
src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosAndroid: Native01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: Native03_iOS_Native.mp4iOS: mWeb Safari04_iOS_Safari.mp4MacOS: Chrome / Safari05_MacOS_Chrome.mp4MacOS: Desktop06_MacOS_Desktop.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.
LGTM.
We did not find an internal engineer to review this PR, trying to assign a random engineer to #39545 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx
Show resolved
Hide resolved
src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx
Outdated
Show resolved
Hide resolved
src/components/HTMLEngineProvider/HTMLRenderers/MentionReportRenderer.tsx
Outdated
Show resolved
Hide resolved
@rlinoz fixed 😌 |
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!
🚀 Deployed to staging by https://github.com/rlinoz in version: 1.4.63-0 🚀
|
@war-in Can you verify internally, not sure if QA team can paste the code in src/pages/home/report/ReportActionsView.tsx under the PopoverReactionList (line 533)? |
@kbecciv As I saw that other PR's related to mentions are deployed as well, you should be able to just type the mention inside composer - sent message should contain the mention and be styled properly. |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
Details
Fixed Issues
$ #39545
PROPOSAL:
Tests
To check the functionality paste the code in
src/pages/home/report/ReportActionsView.tsx
under thePopoverReactionList
(line 533)#admins
room3088255940953979
(you probably would need to change the id to one existing on your account)Offline tests
QA Steps
To check the functionality paste the code in
src/pages/home/report/ReportActionsView.tsx
under thePopoverReactionList
(line 533)#admins
room3088255940953979
(you probably would need to change the id to one existing on your account)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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.-.native.mov
Android: mWeb Chrome
android.-.web.mov
iOS: Native
ios.-.native.mov
iOS: mWeb Safari
ios.-.web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov