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

Prevent bold, italic, strikethrough, and auto link when the content is a codefence #663

Merged

Conversation

bernhardoj
Copy link
Contributor

Fixed Issues

$ Expensify/App#37533

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    The ExpensiMark-HTML-test.js is updated to include the new changes case.

  2. What tests did you perform that validates your changed worked?
    Ran the unit test.
    Testing in App (Android & iOS only):

  • Type these markdowns into composer:
~```a```~
*```b```*
_```c```_
[```d```](google.com)
  • Verify the strikethrough, bold, italic, and auto link isn't applied

to be able to test these changes, you need to apply the code change to react-native-live-markdown-parser.js
image

find all occurrences of includes('<pre>') and replace it with includes('</pre>')

Screen.Recording.2024-03-13.at.15.05.10.mov

QA

  1. What does QA need to do to validate your changes?
  2. What areas to they need to test for regressions?
    Same as Test above

@bernhardoj bernhardoj requested a review from a team as a code owner March 13, 2024 07:57
@melvin-bot melvin-bot bot requested review from Beamanator and removed request for a team March 13, 2024 07:58
@Beamanator Beamanator requested review from puneetlath and removed request for Beamanator March 13, 2024 08:11
@Beamanator
Copy link
Contributor

@puneetlath adding you as reviewer since you're assigned to the linked issue

@puneetlath
Copy link
Contributor

@allroundexperts can you please take the first round of review?

@allroundexperts
Copy link
Contributor

Yes. On it today.

@allroundexperts
Copy link
Contributor

Still reviewing this.

Copy link
Contributor

@allroundexperts allroundexperts left a comment

Choose a reason for hiding this comment

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

Works great!

Screen.Recording.2024-03-18.at.3.08.16.AM.mov
Screen.Recording.2024-03-18.at.3.08.46.AM.mov

@puneetlath puneetlath merged commit 9e47e1f into Expensify:main Mar 18, 2024
5 checks passed
@puneetlath
Copy link
Contributor

Is the next step to open an App PR to apply this expensify-common update?

@bernhardoj
Copy link
Contributor Author

@puneetlath @allroundexperts we would need to update the expensify-common version in react-native-live-markdown, but I think it would make sense to have the same version of expensify-common in App too.

react-native-live-markdown already has the latest version of expensify-common (dc8ea98), except our recently merged commit (9e47e1f). I think we need a help from SWM team to update the package as react-native-live-markdown latest version is 0.1.28 and if we update it, that would be 0.1.29 which is 24 patch version apart from the current version in the App (0.1.5).

I see that this issue will update the react-native-live-markdown to the latest version, but I don't see the App PR yet (@robertKozik cmiiw). I think we can hold for that issue or ask them to include our expensify-common update to react-native-live-markdown.

@puneetlath
Copy link
Contributor

Ok sounds good to me. Mind commenting there?

@bernhardoj
Copy link
Contributor Author

I have commented here. In the meantime, I open a live markdown PR to update the expensify-common Expensify/react-native-live-markdown#242

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.

4 participants