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 parsing markdown for multiline email hyperlink #750

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

skyweb331
Copy link
Contributor

Fixed Issues

$ Expensify/App#43386
This PR is to fix parsing markdown for multiline email hyperlinks

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    Passed all existing test cases and Added one more test case for this purpose.
  2. What tests did you perform that validates your changed worked?
    bump expensify-common version in E/App and react-native-live-markdown to this PR and tested locally

QA

  1. What does QA need to do to validate your changes?
    bump expensify-common version in E/App and try to input multiline email hyperlinks ( i.e. test\ntest ) and see if there's any errors or wrong parsing.

@skyweb331 skyweb331 requested a review from a team as a code owner July 12, 2024 09:06
@melvin-bot melvin-bot bot requested review from marcochavezf and removed request for a team July 12, 2024 09:06
@marcochavezf
Copy link
Contributor

Tagging @hungvu193 for C+ review

@hungvu193
Copy link
Contributor

I'll review this tomorrow

@hungvu193
Copy link
Contributor

@skyweb331 One question, when I inserted line break between content, the email won't be highlighted anymore. I assume that this will be fixed when we apply this change from this PR to our react-native-live-markdown right?

Screen.Recording.2024-07-14.at.22.39.22.mov

@skyweb331
Copy link
Contributor Author

@hungvu193 Yes. But it requires time to investigate. For now, what I can say is that it will be problem from htmlToMarkdown parser...

@hungvu193
Copy link
Contributor

Ok. Please take a look and let's me know the result. I can try to test it tomorrow

@skyweb331
Copy link
Contributor Author

@hungvu193
I checked this bug and it is related to react-native-live-markdown
image
Specifically, this function: https://github.com/Expensify/react-native-live-markdown/blob/9c9769ef004657f1dbe71d3e54b34d50c9d87635/parser/index.ts#L90-L199

I ran this function and it throws following error
image

So do you want me to fix react-native-live-markdown for this?

@hungvu193
Copy link
Contributor

I believe it will be a part of this issue too, so Yes, we need to fix it.

@skyweb331
Copy link
Contributor Author

OK. GOT IT. ( I thought react-native-live-markdown is managed by software-mansion so this should be something they should do??? )

@hungvu193
Copy link
Contributor

OK. GOT IT. ( I thought react-native-live-markdown is managed by software-mansion so this should be something they should do??? )

They will help us review our PR once we create it 😄 . So I think here's the plan, we will try to prioritize this PR then create another PR in live markdown to fix the console error.

@skyweb331
Copy link
Contributor Author

Yes. This PR is working correctly and the live editor issue is because they don't handle parsing correctly...
So how about merging this first and meanwhile let them know there is issue when they break parsed string into tokens...

@hungvu193
Copy link
Contributor

Sure. I'm going to approve this PR.

Copy link
Contributor

@hungvu193 hungvu193 left a comment

Choose a reason for hiding this comment

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

LGTM

@hungvu193
Copy link
Contributor

Yes. This PR is working correctly and the live editor issue is because they don't handle parsing correctly... So how about merging this first and meanwhile let them know there is issue when they break parsed string into tokens...

Please link this PR and the original issue when you create PR in react-native-live-markdown.

@skyweb331
Copy link
Contributor Author

@hungvu193 I fixed in react-native-live-markdown and will create PR there.
image

@hungvu193
Copy link
Contributor

@hungvu193 I fixed in react-native-live-markdown and will create PR there. image

Thanks for your quick work 🚀

@skyweb331
Copy link
Contributor Author

But problem is I must upgrade expensify-common version in react-native-live-markdown after merge this PR. otherwise, my PR there will fail test.
So plan is let's merge this first and then I will create a PR in react-native-live-markdown with bumping expensify-common version to newer version and my updates.

@hungvu193
Copy link
Contributor

Yes. Correct. We need to wait for this PR to be merged first, then we will use new expensify-common version and update it to our live markdown PR.

Copy link
Contributor

@marcochavezf marcochavezf left a comment

Choose a reason for hiding this comment

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

Merging this PR to unblock the other one

@marcochavezf marcochavezf merged commit d547933 into Expensify:main Jul 15, 2024
6 checks passed
Copy link

🚀Published to npm in v2.0.46

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.

None yet

3 participants