Skip to content
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

Render shortened mentions with blue/green outline in live markdown preview #38025

Open
puneetlath opened this issue Mar 10, 2024 · 55 comments
Open
Assignees
Labels
External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2

Comments

@puneetlath
Copy link
Contributor

puneetlath commented Mar 10, 2024

Problem

Our markdown rendering renders mentions with a blue or green outline within the chat body like this:
Screenshot 2024-03-10 at 1 10 07 PM

We also have "live markdown preview" in the composer which is supposed to give a live preview of how the text will be rendered like this:
Screenshot 2024-03-10 at 1 12 53 PM

However, this doesn't currently happen for shortened mentions:
Screenshot 2024-03-10 at 1 18 30 PM

Solution

Add shortened mentions to the live markdown preview rendering that happens in the composer.

Note for potential contributors: live markdown preview currently only exists on mobile apps.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b3ecb2640f3bf7a3
  • Upwork Job ID: 1766890615417413632
  • Last Price Increase: 2024-03-10
  • Automatic offers:
    • shubham1206agra | Contributor | 105292539
@puneetlath puneetlath added External Added to denote the issue can be worked on by a contributor Daily KSv2 NewFeature Something to build that is a new item. labels Mar 10, 2024
@puneetlath puneetlath self-assigned this Mar 10, 2024
@melvin-bot melvin-bot bot changed the title Render mentions with blue/green outline in live markdown preview [$500] Render mentions with blue/green outline in live markdown preview Mar 10, 2024
Copy link

melvin-bot bot commented Mar 10, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01b3ecb2640f3bf7a3

Copy link

melvin-bot bot commented Mar 10, 2024

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

@melvin-bot melvin-bot bot added Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2 and removed Daily KSv2 labels Mar 10, 2024
Copy link

melvin-bot bot commented Mar 10, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 10, 2024
@puneetlath puneetlath changed the title [$500] Render mentions with blue/green outline in live markdown preview [$500] Render shortened mentions with blue/green outline in live markdown preview Mar 10, 2024
@miroshar-success
Copy link

No problem. Done

Copy link

melvin-bot bot commented Mar 11, 2024

📣 @OskarMast! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@brandonhenry
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

New Feature - The live markdown preview in the composer (mobile app only) does not currently render shortened mentions (e.g., @username without the domain) with the blue/green outline like it does for full mentions (e.g., @user@domain.com). This is inconsistent with how mentions actually render in the chat.

What is the root cause of that problem?

The root cause is that the logic for detecting and rendering mentions in the live preview, specifically in the ExpensiMark lib, does not account for the shortened mention format. The regular expressions (REG_EXP.EMAIL_PART and REG_EXP.MARKDOWN_EMAIL) and the userMentions rule only match full mentions with the domain included. As a result, shortened mentions are not detected and rendered with the appropriate styling in the live preview.

What changes do you think we should make in order to solve the problem?

To solve this issue, we should make the following changes:

  1. Update the REG_EXP.EMAIL_PART constant to also match shortened mentions without the domain part:
readonly EMAIL_PART: "([\\w\\-\\+\\'#]+(?:\\.[\\w\\-\\'\\+]+)*(?:@(?:[\\w\\-]+\\.)+[a-z]{2,})?)";
  1. Update the REG_EXP.MARKDOWN_EMAIL constant to match both full and shortened mentions:
readonly MARKDOWN_EMAIL: "([\\w\\-\\+\\'#]+(?:\\.[\\w\\-\\'\\+]+)*(?:@(?:[\\w\\-]+\\.)+[a-z]{2,})?)";

Now, the userMentions rule will handle both full and shortened mentions, thus adding the new feature we're looking for in the markdown live preview.

@shubham1206agra
Copy link
Contributor

Tagging @robertKozik @tomekzaw as this is related to markdown preview.

@tomekzaw
Copy link
Contributor

@thienlnam We would like to work on it as this probably requires some small changes in Live Markdown library as well.

@SzymczakJ
Copy link
Contributor

Hey! I’m Jakub Szymczak from Software Mansion, an expert agency, and I’d like to work on this issue!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 11, 2024
@thienlnam thienlnam self-assigned this Mar 11, 2024
@brandonhenry
Copy link
Contributor

Personally I don't think this requires any change in markdown live preview, and should only include an update to our expensify-common library. See proposal @thienlnam

@puneetlath puneetlath changed the title [HOLD react-native-live-markdown #317] [$500] Render shortened mentions with blue/green outline in live markdown preview Render shortened mentions with blue/green outline in live markdown preview Dec 10, 2024
@puneetlath
Copy link
Contributor Author

Great and will @SzymczakJ still be the person to work on it?

@tomekzaw
Copy link
Contributor

Someone from SWM will handle this task as this requires some extra thoughts on how to pass app state to Markdown parser efficiently. Let me check internally who's the lucky person and I will let you know.

@shubham1206agra
Copy link
Contributor

@puneetlath Can I take this over as a reviewer here?
Since I am C+ assigned in #35725.

@tomekzaw
Copy link
Contributor

Just for the context, we're still waiting for the live-markdown bump to be merged (#53627) so let's hold on that PR for now

@puneetlath puneetlath changed the title Render shortened mentions with blue/green outline in live markdown preview [HOLD #53627] Render shortened mentions with blue/green outline in live markdown preview Dec 11, 2024
Copy link

melvin-bot bot commented Dec 11, 2024

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@tomekzaw
Copy link
Contributor

Someone from SWM will work on this feature

@Kicu
Copy link
Contributor

Kicu commented Dec 12, 2024

Hey guys I work form SWM, please assign me to this issue and I will start working on it.

@allroundexperts
Copy link
Contributor

Can I take this over as a reviewer here?
Since I am C+ assigned in #35725.

@shubham1206agra Do you have greater context here? I see that #35725 is just on hold for this issue. I was tagging along this one since March 😢

@shubham1206agra
Copy link
Contributor

Do you have greater context here?

@allroundexperts Yes since I was following the upstream PR and #35725 is there since February.

@allroundexperts
Copy link
Contributor

@allroundexperts Yes since I was following the upstream PR and #35725 is there since February.

Gotcha.

@Kicu
Copy link
Contributor

Kicu commented Dec 17, 2024

@puneetlath I'm not sure if you're the best person to ask, but I have some questions.

I'm currently looking at the regexes in ExpensiMark used to parse userMention (https://github.com/software-mansion-labs/expensify-common/blob/main/lib/ExpensiMark.ts#L365) and I'm trying to add another regex for shortMention.
We need ExpensiMark to return these shortened version so that later (in a worklet) we can compare them with your onyx data.

What specific versions of short mentions should we support? I'm assuming the most basic one is: @john.doe.
But should I also support:

  • @justOneWord?
  • any special characters?
  • are phone numbers affect short mentions in any way?

I guess I can use the first part of CONST.REG_EXP.EMAIL_PART for this.

@puneetlath
Copy link
Contributor Author

We need to support anything that could be before the @ in an email address. So whatever formatting and special characters are supported for that. I don't think phone numbers matter for short mentions.

@Kicu
Copy link
Contributor

Kicu commented Dec 21, 2024

Before Christmas update.

For the past few days I've been working on ExpensiMark, to try to understand the current email-parsing regexes and to add short-mentions.
Here is a draft PR that shows how this could look like, most of the ExpensiMark.replace tests are passing, there are several edge-cases that need further digging.
Expensify/expensify-common#824

Please take a look at the PR and more specifically at which tests are failing, because some of these are tricky and need some agreeing what we would consider a short mention.

The current plan would be:

  • allow ExpensiMark to understand and parse short mentions, so that the tag <mention-short can be returned
  • use the output of ☝️in parseExpensiMark fn in live-markdown, to be able to create markdown with possible mentions
  • in a worklet, inside E/App pass list of possible mentions (based on users in Onyx) to a parsing function, and verify if any of "possible" mentions is actually a mention, then decorate it

I'm going on Holiday till January 6th, however I should be around to answer any questions around 30/31 Dec.

@Kicu
Copy link
Contributor

Kicu commented Dec 31, 2024

In depth implementation details

In order to implement this task we need to modify 3 (sic!) repos: A/App, expensify-common (because Expensimark is there) and react-native-live-markdown.

List of PRs needed to implement this issue:

Implementation plan

  1. Merge live-markdown PR as it does not conflict with anything - done ✅
  2. Review and merge ExpensiMark PR to allow for parsing of short-mentions
  3. use the output of ☝️in parseExpensiMark function in live-markdown, to be able to create markdown with possible mentions - for this we need to bump expensify-common and live-markdown once both PRs are merged
  4. in a worklet, inside E/App pass list of possible mentions: [WIP] Show current user mention in MarkdownTextInput with green outline #54037

@melvin-bot melvin-bot bot removed the Overdue label Dec 31, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 8, 2025
@thienlnam thienlnam changed the title [HOLD #53627] Render shortened mentions with blue/green outline in live markdown preview Render shortened mentions with blue/green outline in live markdown preview Jan 8, 2025
@thienlnam
Copy link
Contributor

expensify-common PR in review - @shubham1206agra could you also take a look at this PR since you're the C+ here? Expensify/expensify-common#824

Tried to assign you but can't until you comment on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants