feat(mdxish): render newlines for consecutive blocks#1328
feat(mdxish): render newlines for consecutive blocks#1328eaglethrost wants to merge 4 commits intonextfrom
Conversation
There was a problem hiding this comment.
Noticed some uncaught lint errors from next, fixed them here
| .use(rehypeRaw, { passThrough: ['html-block'] }) | ||
| .use(restoreBooleanProperties) | ||
| .use(mdxishMermaidTransformer) // Add mermaid-render className to pre wrappers | ||
| .use(splitNewlines) |
There was a problem hiding this comment.
Decided to do it in the HTML ecosystem, and not in the mdxishAstProcessor stage because:
- It would slightly change the resulting AST and since our editor relies on it, might cause unintended effects. I also think since this is a rendering issue, it doesn't really make sense to put it in the ast processor stage
- Easier to process in the ecosystem
|
|
||
| // Typically this transformer should only be applied to text inside p and li tags | ||
| // where the problem occurs. For other tags, we bias to leave the behavior as is. | ||
| const INCLUDED_PARENTS = new Set(['p', 'li']); |
There was a problem hiding this comment.
To avoid transforming under areas that shouldn't be touched (e.g. code blocks, inline code), and to avoid regressions, I decided to limit under which tags does this apply to. From my understanding this new line rendering issue almost all of the times happen under paragraphs, and sometimes list. But I might have missed other elements (let me know if you can think of something else)
| // transform it | ||
| if (typeof node.value !== 'string' || !node.value.trim() || !node.value.includes('\n')) return; | ||
|
|
||
| // Splint text by \n into different nodes, with the \n being converted to <br> element |
There was a problem hiding this comment.
| // Splint text by \n into different nodes, with the \n being converted to <br> element | |
| // Split text by \n into different nodes, with the \n being converted to <br> element |
typo?
kevinports
left a comment
There was a problem hiding this comment.
Tried to think of any test cases we might want to add. But the logic looks good to me!
There was a problem hiding this comment.
A few edge case tests we might want to cover:
- Double new lines
Line 1\n\nLine 2should render<p>Line 1</p> <p>Line 2</p> - Trailing new line
Line 1\nshould render<p>Line 1</p> - Leading new line
\nLine 1should render<p>Line 1</p>
| * This transformer would allow consecutive lines in markdown to be rendered as separate lines | ||
| * without the need for the empty line. | ||
| */ | ||
| const splitNewlines = () => (tree: Root) => { |
There was a problem hiding this comment.
@eaglethrost do you mind writing up a quick comparison/reasoning for this approach versus the way we used to do this in Legacy per my comment here?
|
going to close this PR in favour of doing them together over in #1329 |
🧰 Changes
\ncharacters that get bunched by remark in a text node, and parses and splits the node with a<br>tag so that newlines get rendered🧬 QA & Testing