[HOLD for payment 2023-05-16] Add custom renderer for user mentions

[puneetlath]

As part of the mentions project, we will be adding a custom renderer called MentionUserRenderer for the tag. The custom renderer will be similar to the AnchorRenderer.

The details of the implementation are described in the doc here.

[MelvinBot]

Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new.

[robertKozik]

Hi 👋🏼 I'm from Software Mansion and I'll handle this task

[puneetlath]

Ok great @robertKozik. I have assigned you to the issue and shared the design doc with you. Please feel free to hit me up here or in slack if you have any questions!

[puneetlath]

@robertKozik if you want to test this, the easiest way will be to:

1. Add the changes from this PR in App/node_modules/expensify-common/lib/ExpensiMark.js. That will make it so that when you type @here or @user@domain.com it gets the <mention-here> or <mention-user> html tags added.
2. Force the app offline. This way when you send a message it'll just show the optimistic message, not the one returned by the server. This is because the server will strip any html tags it doesn't recognize (we just merged a PR to allow these two, so it won't be live for another few days)

[robertKozik]

Thats way more convenient than what I used to do (patching the ReportActionItemFragment to replace all occurances). Thanks!

[robertKozik]

Hey Hey, two follow up questions:

1. In the mockups in design doc the tooltip which turn on after hovering the user mention includes user profile image. The snippet of userMentionRenderer does not include it. Is it still in the scope for the Before EC3 version?
2. User mentions which comes from the ExpensiMark does not include fields like login (as stated in design doc), it's only: <mention-user>@user@mail</mention-user> Is it intended and the login handle is only provided inside the body of the HTML tag, or It's just due to that I've only applied changes inside ExpensiMark ?

[puneetlath]

Great questions.

1. Given that we're using primary logins, I think we can skip the tooltip altogether for the before EC3 portion of mentions. I don't think any tooltip is needed. We just need to make sure that the profile page is opened when you click on a mention.

2. Yes, for now there won't be a login attribute. There won't be any html attributes. Just the text between the tags.

[robertKozik]

Thanks for answers, today I should create the PR with it.

[puneetlath]

Awesome!

[robertKozik]

Hi! Sadly I didn't finish it :/ I've encountered two problems:

1. When I've patched the ExpensiMark file, on Safari browser app is not loading for me and gets following error. Looks like safari can't handle new regex, but I'm not sure about it.

2. Android native app can't render mention when it's wrapped in Pressable component. Only after refreshing the code, mentions are appearing

I'll investigate it further @puneetlath

[puneetlath]

Hmm, interesting. It looks like our custom renderer for Anchor tags also returns a component wrapped in a Pressable.

[getusha]

When I've patched the ExpensiMark file, on Safari browser app is not loading for me and gets following error. Looks like safari can't handle new regex, but I'm not sure about it.

Hi @robertKozik
It seems like negative look behind regex group is not supported on safari browser yet, i will investigate and update the PR with the necessary changes.

[puneetlath]

@robertKozik I just wanted to check in and see how this is going.

[robertKozik]

Sorry, I was OOO most of the time. I've checked the android issue on the physical device and now it work as it should be, maybe that was a problem with my simulator. I'll check today one more just to be sure, and tomorrow PR will be ready to review, hopefully alongside the @here mention renderer as it is very similar

[puneetlath]

Awesome!

[robertKozik]

I've pushed PR to review. Problems with android were indeed corelated with usage of Pressable component. The Anchor renderer uses the PressableWithSecondaryInteraction with inline prop – it determines usage of Text rather than Pressable, as it causes problem with positioning of rendered fragment.

I've used newest implementation of ExpensiMark changes, it works on every browser, although now it cannot recognise properly when mention handle has the . before the at sign @. F.E. my mail: @robert.kozik@swmansion.com renders as mail rather than mention. Is it intended?
CC. @puneetlath @getusha

[puneetlath]

Nice! Reviewing asap.

Yes, it is intended that only certain characters are allowed directly before the @ sign in a mention and . is not one of them.

[puneetlath]

Just merged.

[robertKozik]

Great! I'll follow up with the here mention renderer soon

[robertKozik]

As the PR needs to be reverted due to android problems, what is the correct way to go with this? Do we pivot to the non-rounded mentions for now?

[puneetlath]

Ok I reverted the PR. This issue is the problem.

I'm going to chat internally and see what the best path forward is here.

[robertKozik]

That catch me off guard 😢 I could have sworn that It was working for me while testing...

Let me know if you will know what next, with the work we do already here reverting to the more simple colouring will be an ease and would take no time

[parasharrajat]

@puneetlath Please assign this issue to me as C+.

I think we can just use no border-radius and padding solution for now. When the inline-code block PR is fixed, we can swap that.

[getusha]

That catch me off guard 😢 I could have sworn that It was working for me while testing...

Let me know if you will know what next, with the work we do already here reverting to the more simple colouring will be an ease and would take no time

Is there any alternative of using InlineCodeBlock? it's the only issue right now. the issue is unpredictable it works sometimes but won't work suddenly, but most of the time it's not working.

[puneetlath]

Ok @robertKozik @parasharrajat I chatted internally and let's go back to using styled text so it will look good on most devices and just accept that it will be ugly on Android in some scenarios.

So back to what you had originally that had this problem:

And then when Expensify/react-native#43 is merged, the issue should resolve itself.

[puneetlath]

Sorry for all the flip-flopping!

[robertKozik]

Alright, It will hopefully need only reverting commits, so I'll do it right away, create new PR and test it

And then when Expensify/react-native#43 is merged, the issue should resolve itself.

Not quite itself, as it will introduce new prop for such styling. But follow-up pr would need only to move style to another prop.

[puneetlath]

Sounds great and that makes sense. Thanks @robertKozik

[robertKozik]

Created draft PR for that, just want to check it on all platforms and I'll change it to ready cc. @parasharrajat

[parasharrajat]

I will check this tomorrow daytime. Night..Night...

[parasharrajat]

PR is not yet ready for review.

[robertKozik]

Sorry, forgot to change it 😄

[puneetlath]

Merged again!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.12-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [No QA] 17885 custom user mentions renderer #18167
• [No QA] create UserMentionRenderer for rendering mentions inside chat #18495

If no regressions arise, payment will be issued on 2023-05-16. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[puneetlath]

@parasharrajat just sent you a contract offer: https://www.upwork.com/nx/wm/pre-hire/c/8577561/offer/24575855

[puneetlath]

All paid. Thanks everyone!