-
Notifications
You must be signed in to change notification settings - Fork 75
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
chore: fix ansi escaping for diffs #557
Conversation
pavelfeldman
commented
Nov 14, 2024
5a4a82c
to
550610d
Compare
const isItalic = style['font-style'] === 'italic'; | ||
if (isItalic) | ||
token = `<i>${token}</i>`; | ||
const hasOpacity = style['opacity'] === '0.8'; |
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.
What is so special about 0.8?
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.
It is assigned in the line 34, the idea is that we use upstream code for style definition, but then we sanitize it for vscode to accept it.
const isBold = style['font-weight'] === 'bold'; | ||
if (isBold) | ||
token = `<b>${token}</b>`; | ||
const isItalic = style['font-style'] === 'italic'; |
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.
Let's have a type for style
and also just check if style.italic is true?
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.
That would fork the code from the upstream a lot.
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.
This part is already different
const hasOpacity = style['opacity'] === '0.8'; | ||
if (hasOpacity) | ||
token = `<span style='color:#666;'>${token}</span>`; | ||
const color = reverse ? (bg || '#000') : fg; |
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.
I believe default background will depend on dark/light mode.
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.
Yeah, I checked it in both modes and it is ok because we only do reverse on mid-light background.
|
||
function escapeHTML(text: string): string { | ||
return text.replace(/[&"<>\s\n]/g, c => ({ | ||
' ': ' ', |
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.
this won't match other whitespaces, such as \t. Also, do we want to map all whitespaces to non-breaking?
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.
This has not changed
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.
hmm, this is what I see upstream:
function escapeHTML(text: string): string {
return text.replace(/[&"<>]/g, c => ({ '&': '&', '"': '"', '<': '<', '>': '>' }[c]!));
}
550610d
to
050c5c5
Compare