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

Add support for pasting all supported inline tags #90

Closed
wants to merge 19 commits into from

Conversation

iansan5653
Copy link
Member

As discussed in #67, this PR adds support for keeping the formatting provided by supported inline HTML elements.

This includes the following:

HTML Markdown Result
not bold <strong>bold</strong> not bold not bold **bold** not bold not bold bold not bold
not italic <em>italic</em> not italic not italic *italic* not italic not italic italic not italic
not strikethrough <del>strikethrough</del> not strikethrough not strikethrough ~~strikethrough~~ not strikethrough not strikethrough strikethrough not strikethrough
not code <code>code</code> not code not code `code` not code not code code not code
not kbd <kbd>kbd</kbd> not kbd not kbd <kbd>kbd</kbd> not kbd not kbd kbd not kbd
not sup <sup>sup</sup> not sup not sup <sup>sup</sup> not sup not sup sup not sup
not sub <sub>sub</sub> not sub not sub <sub>sub</sub> not sub not sub sub not sub
not ins <ins>ins</ins> not ins not ins <ins>ins</ins> not ins not ins ins not ins

I left out the block tags from the issue because these require special handling (adding line breaks) and I didn't want to make this PR too big.

@iansan5653 iansan5653 requested a review from a team as a code owner September 18, 2023 19:36
src/markdown.ts Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Created this new file to standardize the building of the Markdown strings.

Note: I had to disable lint errors because we are not escaping the contents. This is consistent with existing building of link/image/HTML strings, but an argument could be made that we should escape the contents. Not for XSS aversion (this text is going into a textarea so it won't be run directly) but just to make the output more versatile to complex input. But this is more difficult than just HTML escaping - we would also need to escape Markdown special characters... I decided to leave this for the future.

@@ -95,6 +106,29 @@ function convertToMarkdown(plaintext: string, walker: TreeWalker): string {
return index === NODE_LIMIT ? plaintext : markdown
}

function getNodeMarkdownBuilder(node: Node): (() => string) | void {
Copy link
Member Author

Choose a reason for hiding this comment

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

This function returns a function to make it lazy. This allows us to see if we support converting a specific Node without actually spending the energy to convert it.

@@ -95,6 +106,29 @@ function convertToMarkdown(plaintext: string, walker: TreeWalker): string {
return index === NODE_LIMIT ? plaintext : markdown
}

function getNodeMarkdownBuilder(node: Node): (() => string) | void {
if (node instanceof HTMLAnchorElement) return () => linkify(node)
Copy link
Member Author

Choose a reason for hiding this comment

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

links must be specialcased to get access to the href property on the instance

@@ -62,9 +73,7 @@ function convertToMarkdown(plaintext: string, walker: TreeWalker): string {
// Walk through the DOM tree
while (currentNode && index < NODE_LIMIT) {
index++
const text = isLink(currentNode)
? (currentNode.textContent || '').replace(/[\t\n\r ]+/g, ' ')
Copy link
Member Author

Choose a reason for hiding this comment

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

This line break stripping is still done, just later in the process

@primer-css
Copy link

👋 Hello and thanks for pinging us! This issue or PR has been added to our inbox and a Primer first responder will review it soon.

  • 🎨 If this is a PR that includes a visual change, please make sure to add screenshots in the description or deploy this code to a lab machine with instructions for how to test.
  • If this is a PR that includes changes to an interaction, please include a video recording in the description.
  • ⚠️ If this is urgent, please visit us in #primer on Slack and tag the first responders listed in the channel topic.

@iansan5653 iansan5653 marked this pull request as draft September 19, 2023 16:04
@iansan5653 iansan5653 closed this Sep 25, 2023
@iansan5653
Copy link
Member Author

Didn't manage to make it work and won't have capacity to revisit for a while, so I'm going to close this out.

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.

2 participants