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

Fix problem with parsing links containing mailto: prefix followed by an email #604

Conversation

Skalakid
Copy link
Contributor

@Skalakid Skalakid commented Nov 15, 2023

This PR fixes potential problems with parsing links containing mailto: prefix followed by an email. The changes were added to enhance parsing inside Live Markdown Preview.
The problem was mainly with parsing [mailto:[email protected]](mailto:[email protected]). ExpensiMark instead of one link returns two separate tags inside brackets:

[<a href=\"mailto:[email protected]\">[email protected]</a>](mailto:<a href=\"mailto:[email protected]\">[email protected]</a>)

To fix the problem we decided to collapse such links, similarly like it is done after writing
[[email protected]]([email protected]).

So now following strings:

will be collapsed into: <a href=\"mailto:[email protected]\">[email protected]</a>

mailto.parsing.mov

Fixed Issues

$ Expensify/App#31181

Tests

To test these changes I've added proper tests connected to all needed behaviors for Live Markdown Preview. To test it by yourself you should:

  1. Run the NewDot/App
  2. Open any chat
  3. Send message that contains:
  1. Verify if every string is collapsed into just [email protected]

QA

Same as tests

Copy link
Contributor

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding proper unit tests.

Comment on lines 71 to 76
let label = g1.trim();
const href = `mailto:${g2}`;
if (label === href) {
label = g2;
}
return `<a href="${href}">${label}</a>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the fact that we mutate label here but honestly I cannot think of anything better that wouldn't require duplicating <a href="..."> string.

@Skalakid Skalakid marked this pull request as ready for review November 16, 2023 16:36
@Skalakid Skalakid requested a review from a team as a code owner November 16, 2023 16:36
@melvin-bot melvin-bot bot requested review from jasperhuangg and removed request for a team November 16, 2023 16:37
@Skalakid
Copy link
Contributor Author

@jasperhuangg bump

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this was OOO

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