-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: support accountid attribute in user mentions #26877
Conversation
@thesahindia 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] |
@thesahindia One thing I would like to note is that the self-closing syntax, i.e.: |
Oh, hm, that's interesting. So the back-end will have to return it as Is that specifically a restriction of the renderHTML library we're using? |
It's on the HTML specification level so the default behavior should be the same across renderers. It's explained quite well here: https://developer.mozilla.org/en-US/docs/Glossary/Void_element#self-closing_tags For the There is a prop htmlParserOptions.recognizeSelfClosing that can be set true (reference). It seems to work well in testing. Please note this would be a non-specific change, i.e. it would allow self-closing of other tags as well. If you'd like me to pursue, I will incorporate this here, and I will update the tests to cover the extra scenarios involved. There are two usages of the htmlparser2 library where this prop can also be added, however these involve extracting attachments from reports (extractAttachmentsFromReport) or deal with the browser DOM (getHTMLOfSelection), and it would not be relevant there. |
Ok, got it! Thanks for the detailed explanation! Let's go with the way you have it for now. If we want to add the |
@samh-nl now that the merge freeze is over, please merge main into your branch. |
I have performed the merge. |
Ok great. @thesahindia let us know when you'll be able to review. |
I will review it today. |
@thesahindia bump. |
Hey @puneetlath, sorry for the delay here. I am having some issues with my system and won't be able to look into it soon. Please reassign. |
@situchan bump! |
Mostly looks good.
@puneetlath in this case, should we disable navigation to open detail page? Screen.Recording.2023-09-15.at.11.51.34.PM.mov |
I am not sure if this is possible case: Screen.Recording.2023-09-16.at.12.05.34.AM.mov |
Yes, if we have the accountID in the mention itself, then we can still open the profile page. Only if for some reason there isn't an accountID present, we shouldn't open the profile page.
Let's handle it. It shouldn't happen, but let's handle it in case it does. |
So what I was asking about the case when accountID exists in mention itself but not exist in personalDetailsList.
So this is the case when neither accountID nor email exists in mention tag. What would be the expected behavior here? |
@samh-nl let's address feedback above |
Yes, let's show @hidden in that scenario and still open the profile page. The profile page should already handle the scenario where the accountID isn't present in personalDetails and will also call the API to get whatever info it can. |
Render nothing makes sense to me for this scenario. |
Good questions. |
Ok I will change it, the current implementation is based on #26306 (comment). |
I checked that discussion too but it seems odd to me that it's not actionable as mention is colored always. |
I've merged main into the branch and updated the test steps/videos. |
Bump |
in a few hrs |
@situchan will you be able to get to this today? |
Sometimes infinite loading Screen.Recording.2023-09-21.at.3.31.52.PM.movAlso it doesn't make sense to message Hidden Screen.Recording.2023-09-21.at.3.35.12.PM.mov |
I think they can be out of scope as not caused by this PR |
Being able to message a Hidden user might make sense if the user is offline and we're unable to determine if the account ID really exists. |
I agree that both of those seem out of scope for this PR. |
Server is down at the moment - https://expensify.slack.com/archives/C01GTK53T8Q/p1695314249132269 |
Let's try to get this across the finish line today please! |
Long press on mention doesn't open context menu on native. Works on mWeb |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
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.
@puneetlath all yours!
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!
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.74-0 🚀
|
Are these QA steps?
|
@kavimuru actually this will be basically impossible for you to QA, so you can check it off. As long as mentions are continuing to work as expected in the regression tests, we should be good. |
Hey @puneetlath @samh-nl. Not sure if you're already aware of this, but I'm not sure if we updated the report previews to support the new accountID based mentions. Is this something on your radar? |
@jasperhuangg I believe in this case we just render In addition to |
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.74-3 🚀
|
Details
This PR adds support for the account ID to be provided in user mentions.
Usage examples:
<mention-user accountID=12345678>@test@test.com</mention-user>
or<mention-user accountID=12345678></mention-user>
Fixed Issues
$ #26306
PROPOSAL: #26306 (comment)
Tests
Precondition: The API does not yet support this feature. To allow testing, we can perform a modification here to automatically add the mentions to messages:
In the above, change:
Verify precondition
Tests
Offline tests
Identical to "Tests" section.
QA Steps
Identical to "Tests" section.
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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Web.Screen.Recording.2023-09-19.at.11.41.49.mp4
Mobile Web - Chrome
mWeb.Chrome.Screen.Recording.2023-09-19.at.13.46.33.mp4
Mobile Web - Safari
mWeb.Safari.Screen.Recording.2023-09-19.at.12.44.48.mp4
Desktop
Desktop.Screen.Recording.2023-09-19.at.11.57.37.mp4
iOS
Native.iOS.Screen.Recording.2023-09-19.at.12.23.22.mp4
Android
Native.Android.Screen.Recording.2023-09-19.at.13.38.39.mp4