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

End the url at the first unmatched closing parenthesis #578

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

ShogunFire
Copy link
Contributor

@robertKozik @cead22
We end the url at the first unmatched parenthesis because it is unlikely that this closing parenthesis is part of the url and it can be part of the markdown for example

Fixed Issues

$ Expensify/App#26788

Tests

  1. Go to any report
  2. Send a link with multiple umatched closing parentheses that are not all at the end, for example: google.com/(toto))titi)
  3. Verify that the link ends at the first unmatched parenthesis, in the above example the link should be google.com/(toto)

QA

Same tests and test urls and markdown in general

Video

2023-09-19.09-28-20.mp4

…url at the first one we encounter

Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
@ShogunFire
Copy link
Contributor Author

@robertKozik

@neil-marcellini neil-marcellini requested review from cead22 and removed request for neil-marcellini September 20, 2023 03:43
@neil-marcellini
Copy link
Contributor

@cead22 I'm going to let you take this one since you're the assigned engineer on the issue.

__tests__/ExpensiMark-HTML-test.js Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
@ShogunFire
Copy link
Contributor Author

@cead22 I made the changes

cead22
cead22 previously approved these changes Sep 26, 2023
@cead22
Copy link
Contributor

cead22 commented Sep 26, 2023

@ShogunFire lint errors

Signed-off-by: Pierre Michel <pmiche04@gmail.com>
@ShogunFire
Copy link
Contributor Author

They should be fixed now but I can't verify because it doesn't launch the test

@cead22 cead22 merged commit 82c45d6 into Expensify:main Sep 28, 2023
3 checks passed
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