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

add mention user rule #600

Closed
wants to merge 3 commits into from
Closed

Conversation

tienifr
Copy link
Contributor

@tienifr tienifr commented Nov 10, 2023

Fixed Issues

$ Expensify/App#29978

Tests

  1. Go to staging.new.expensify.com
  2. Create a room
  3. Click on the room header > Members > Invite > Invite anyone to the room
  4. Right click on the system invitation message > Copy to clipboard
  5. Paste the message in the composer
  6. Verify the system message along with the user mention will be copied

QA

  1. Go to staging.new.expensify.com
  2. Create a room
  3. Click on the room header > Members > Invite > Invite anyone to the room
  4. Right click on the system invitation message > Copy to clipboard
  5. Paste the message in the composer
  6. Verify the system message along with the user mention will be copied

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.2023-11-10.at.15.50.29.mov
iOS: mWeb Safari
Screen.Recording.2023-11-10.at.15.34.11.mov
MacOS: Chrome / Safari
Screen.Recording.2023-11-10.at.11.56.25.mov
MacOS: Desktop
Screen.Recording.2023-11-10.at.15.42.12.mov

@tienifr
Copy link
Contributor Author

tienifr commented Nov 10, 2023

@ArekChr The PR is ready for review. For htmlToMarkdownRules, I create 2 userMention rules because we handle copy html to clipboard for Safari and other platform are different

@@ -57,13 +66,14 @@ export default class ExpensiMark {
*
* @param htmlString
*/
htmlToMarkdown(htmlString: string): string;
htmlToMarkdown(htmlString: string, personalDetails: PersonalDetails): string;
Copy link

@ArekChr ArekChr Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be treated as metadata, allowing us to pass various data through the second parameter.

E.g.

const metadata = { personalDetails }
htmlToMarkdown(html, metadata)
const metadata = { reportDetails }
htmlToMarkdown(html, metadata)

same in htmlToText.
WDYT?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the second parameter be optional?

Suggested change
htmlToMarkdown(htmlString: string, personalDetails: PersonalDetails): string;
htmlToMarkdown(htmlString: string, personalDetails?: PersonalDetails): string;

@tienifr
Copy link
Contributor Author

tienifr commented Nov 15, 2023

@ArekChr Thanks for your suggestion, I've updated

@roryabraham
Copy link
Contributor

Looks like we can close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants