Skip to content

Delete nested tag inside codeFence#564

Merged
puneetlath merged 5 commits into
Expensify:mainfrom
akamefi202:selection-copy-markdown
Aug 14, 2023
Merged

Delete nested tag inside codeFence#564
puneetlath merged 5 commits into
Expensify:mainfrom
akamefi202:selection-copy-markdown

Conversation

@akamefi202

Copy link
Copy Markdown
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

github-actions Bot commented Aug 9, 2023

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

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

@jasperhuangg

Copy link
Copy Markdown
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
Copy Markdown
Contributor

@mollfpr please take the first review.

@mollfpr

mollfpr commented Aug 11, 2023

Copy link
Copy Markdown
Contributor

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.

@akamefi202
akamefi202 force-pushed the selection-copy-markdown branch from dab7b15 to eddbc14 Compare August 11, 2023 06:18
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
Signed-off-by: akamefi202 <akamefi202@gmail.com>
@akamefi202
akamefi202 force-pushed the selection-copy-markdown branch from eddbc14 to 23a1a44 Compare August 11, 2023 06:21
Comment thread package-lock.json

Copy link
Copy Markdown
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
Copy Markdown
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.

@mollfpr mollfpr left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@akamefi202
akamefi202 deleted the selection-copy-markdown branch August 14, 2023 17:46
@akamefi202

Copy link
Copy Markdown
Contributor Author

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

@akamefi202

Copy link
Copy Markdown
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