-
Notifications
You must be signed in to change notification settings - Fork 135
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
Delete nested tag inside codeFence #564
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@puneetlath assigning you since this is related to an issue you're managing |
@mollfpr please take the first review. |
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 |
dab7b15
to
eddbc14
Compare
Signed-off-by: akamefi202 <[email protected]>
Signed-off-by: akamefi202 <[email protected]>
Signed-off-by: akamefi202 <[email protected]>
Signed-off-by: akamefi202 <[email protected]>
Signed-off-by: akamefi202 <[email protected]>
eddbc14
to
23a1a44
Compare
package-lock.json
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did.
There was a problem hiding this 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>'; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah cool, I see.
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 @mollfpr Thanks. I will create a PR in the App repo too! |
@puneetlath @mollfpr I created the PR in App repo. |
Added a new rule to
htmlToMarkdownRules
to remove unnecessary nested tags inside<pre>
tag.Fixed Issues
Expensify/App#23659
Tests
In
ExpensiMark-Markdown-test.js
file, test namedTest codeFence copy from selection does not add extra new line
.Checked if
htmlToMarkdown
removes unnecessary tags correctly so extra newline character isn't added.QA
Run the new test cases.
N/A