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

Delete nested tag inside codeFence #564

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

akamefi202
Copy link
Contributor

Added a new rule to htmlToMarkdownRules to remove unnecessary nested tags inside <pre> tag.

Fixed Issues

Expensify/App#23659

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
    In ExpensiMark-Markdown-test.js file, test named Test codeFence copy from selection does not add extra new line.
  2. What tests did you perform that validates your changed worked?
    Checked if htmlToMarkdown removes unnecessary tags correctly so extra newline character isn't added.

QA

  1. What does QA need to do to validate your changes?
    Run the new test cases.
  2. What areas to they need to test for regressions?
    N/A

@akamefi202 akamefi202 requested a review from a team as a code owner August 9, 2023 07:07
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from jasperhuangg and removed request for a team August 9, 2023 07:08
@akamefi202
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@jasperhuangg
Copy link
Contributor

@puneetlath assigning you since this is related to an issue you're managing

@jasperhuangg jasperhuangg removed their request for review August 9, 2023 17:16
@puneetlath
Copy link
Contributor

@mollfpr please take the first review.

@mollfpr
Copy link
Contributor

mollfpr commented Aug 11, 2023

Sorry for the delay guys. I got cold yesterday 🙏

@akamefi202 You need to sign the commit https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#begin-coding-your-solution-in-a-pull-request

For the CLA, I guess you can comment recheck for the action to rerun.

Signed-off-by: akamefi202 <[email protected]>
Signed-off-by: akamefi202 <[email protected]>
Signed-off-by: akamefi202 <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

@akamefi202 Could you revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did.

Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

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

Tested great on the ND and seems the tests are run pretty well 👍

testString = '<pre>code<br></pre>text';
expect(parser.htmlToMarkdown(testString)).toBe('```\ncode\n```\ntext');

testString = '<h3>test heading</h3><div><pre class=\"notranslate\"><code class=\"notranslate\">Code snippet\n</code></pre></div><blockquote><p><a href=\"https://www.example.com\">link</a></p></blockquote>';
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why are we using an h3 in this test instead of an h1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we copy & paste selection from Github, heading is shown as h3 tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool, I see.

@puneetlath
Copy link
Contributor

Looks good to me! @akamefi202 you'll next need to open a PR in the App repo to point to the updated expensify-common commit hash.

@puneetlath puneetlath merged commit 4cc5f72 into Expensify:main Aug 14, 2023
3 checks passed
@akamefi202 akamefi202 deleted the selection-copy-markdown branch August 14, 2023 17:46
@akamefi202
Copy link
Contributor Author

@puneetlath @mollfpr Thanks. I will create a PR in the App repo too!

@akamefi202
Copy link
Contributor Author

@puneetlath @mollfpr I created the PR in App repo.
Expensify/App#24549

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