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

IconLabel markdown title debt #109914

Merged
merged 3 commits into from
Nov 5, 2020
Merged

IconLabel markdown title debt #109914

merged 3 commits into from
Nov 5, 2020

Conversation

alexr00
Copy link
Member

@alexr00 alexr00 commented Nov 3, 2020

Fixes #109231

@@ -301,3 +301,73 @@ function getInsaneOptions(options: { readonly isTrusted?: boolean }): InsaneOpti
};
}

export function RenderMarkdownAsPlaintext(markdown: IMarkdownString) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@mjbvz would you be able to review this function?

@alexr00 alexr00 requested a review from bpasero November 3, 2020 15:09
@alexr00 alexr00 self-assigned this Nov 3, 2020
@alexr00 alexr00 added this to the November 2020 milestone Nov 3, 2020
@bpasero bpasero requested a review from mjbvz November 3, 2020 16:40
src/vs/base/browser/markdownRenderer.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/iconLabel/iconLabel.ts Outdated Show resolved Hide resolved
src/vs/base/browser/ui/iconLabel/iconLabel.ts Outdated Show resolved Hide resolved
@alexr00
Copy link
Member Author

alexr00 commented Nov 4, 2020

@bpasero I think I have addressed your feedback.

@@ -301,3 +301,73 @@ function getInsaneOptions(options: { readonly isTrusted?: boolean }): InsaneOpti
};
}

export function renderMarkdownAsPlaintext(markdown: IMarkdownString) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this function is meant to strip all the markup from the string? So # Header would just be rendered as Header?

Just want to check because we also have logic to escape markdown content:

// escape markdown syntax tokens: http://daringfireball.net/projects/markdown/syntax#backslash
That would render # Header as # Header

The change itself looks good. If renderMarkdownAsPlaintext is the right approach, then just add a doc comment on it so its behavior is more clear

Copy link
Member Author

@alexr00 alexr00 Nov 5, 2020

Choose a reason for hiding this comment

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

Added documentation!

if (value.length > 100_000) {
value = `${value.substr(0, 100_000)}…`;
}
return marked.parse(value, { renderer });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just in case, the output should be passed through our html sanitizer (insane) too

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Added a call to sanitizeRenderedMarkdown.

@alexr00 alexr00 merged commit b0a7c84 into master Nov 5, 2020
@alexr00 alexr00 deleted the alexr00/issue109231 branch November 5, 2020 10:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IconLabel markdown title debt
3 participants