-
Notifications
You must be signed in to change notification settings - Fork 135
notebook markdown rendering improvements #10958
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
Conversation
|
E2E Tests 🚀 |
Prevent span elements from being created for each newline character in the html string returned by the markdown parser. These span elements should not exist in the table markup which is causing css rules to not work as expected.
When there are multiple tables back to back the edges are butted up against each other. We don't want table elements right up against other elements so we need to add some margin.
543b798 to
80edd61
Compare
seeM
left a comment
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 looks great ty!
nstrayer
left a comment
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.
All looks good to me and the example content renders much better now.
I'm surprised these edge cases exist but glad you got to the bottom of them!
If we need to add much more logic in here in the future we should probably see if there's a way we can rethink the whole problem.
| /** | ||
| * Quick lookup table for table elements. | ||
| */ | ||
| const tableElements: Record<string, boolean> = { | ||
| 'table': true, | ||
| 'thead': true, | ||
| 'tbody': true, | ||
| 'tfoot': true, | ||
| 'tr': true, | ||
| 'colgroup': 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.
What about using a set. E.g.
const tableElements = new Set([
'table',
'thead',
'tbody',
'tfoot',
'tr',
'colgroup'
]);Then you can do:
tableElements.has(node.name)Probably actually has worse performance because of how javascript handles short objects with string keys but maybe is more clear?
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.
NVM I see that it's copying the pattern from above. No need to change!
I agree! I think the core of the issue is that we turn every whitespace character into a span. Normally, newlines in html strings are okay because they get collapsed. I don't think this is the ideal fix but the other options are a heavier lift. Something to improve in the future! |
Addresses #10291
This updates some of the markdown rendering styles to better match up with the styles used in other notebooks.
What did get fixed:
What did not get fixed:
NOTE: text content spanning multiple lines works differently for positron notebooks than built-in notebooks: Our markdown rendering logic converts single newlines to spaces instead of treating them as line breaks. Built-in notebooks treat newlines as line breaks. This can be changed but I've left it as is for now. This means that for sentences in block quotes to be on separate lines you need to do:
instead of:
Code Blocks Whitespace Fix
Understanding the cause of the extra whitespace at the bottom of code blocks in Positron notebooks was kind of a pain. I'm including an explanation in case someone has a better suggestion. My fix included modifying the code block renderer in
markdownRenderer.tsbut I don't know why the newline was added in the first place, so maybe there's a better solution?The root cause of the extra whitespace at the bottom of code blocks in Positron notebooks is caused by:
markdownRenderer.ts:111adding a trailing\nto code blocks when rendering them to HTMLrenderHtml.tsx:75which wraps ALL text nodes (including newlines and spaces) in<span>elementsThe
renderHtmlfunction inrenderHtml.tsxlooks like it intentionally preserves all whitespace text nodes because they can be "semantically significant" but in this case the trailing\ninside the<span>is not significant and causes layout issues.Code blocks are
<code>wrapped in a<pre>. The<pre>element has built-inwhite-space: prestyle, which preserves the\ninside the<span>, causing visible whitespace at the bottom of code blocks.Built-in notebooks don't have this issue because they use
markdown-itinstead ofmarkedfor markdown rendering which doesn't add the trailing\nto code blocks from what I can tell.Table Rendering Fix
Similar to the code block styling issue with newlines; the html returned by
markdownRenderer.tsincludes newlines between all the table markup. The html looks something like:When we go to parse this html prior to rendering (
htmlParser.ts:307), we would keep all of these newlines. These newlines then got wrapped in<span>elements. Having<span>elements between the table specific elements breaks the table html spec rules.Per HTML specification:
tablecan only contain:caption,colgroup,thead,tbody,tfoot,trthead,tbody,tfootcan only contain:trtrcan only contain:td,thBreaking this spec prevented existing css from working on tables, such as
border-collapse: collapse, which prevents double borders.To fix this, we now filter out whitespace-only text nodes that are between the table elements listed above when we parse the html string.
AFTER
Untitled.mov
Release Notes
New Features
Bug Fixes
QA Notes
@:positron-notebooks
For testing, I used the following markdown: