[Renderer] Refactor how the markdown renderer works with paragraphs and widgets to be semantically correct#3875
Open
mark-fitzgerald wants to merge 7 commits into
Conversation
…stion-paragraph handler that only includes the JIPT attributes and containing <div> when the JIPT information is available. Override the "paragraph" rule so that it renders a containing <p> element instead of a <div class="paragraph">. When rendering the paragraph, verify that it doesn't have any block-type widgets within it, otherwise, just render the child nodes without a container.
…getBlock rule instead of evaluating paragraph content.
…'main' into LEMS-4282-refactor-paragraph-and-widget-dom-rendering
…break out non-inline widgets out of paragraph containers.
… "special" widgets that are inline, but can't be in a paragraph element. These are rendered without a container. Refactor the code that handles splitting out block-level widgets within apparent inline containers.
…t): [Renderer] Refactor how the markdown renderer works with paragraphs and widgets to be semantically correct
Contributor
|
Size Change: +822 B (+0.16%) Total Size: 518 kB 📦 View Changed
ℹ️ View Unchanged
|
…ed export designations in the util file. Remove files for local dev use.
benchristel
reviewed
Jul 8, 2026
Comment on lines
+1477
to
+1495
| describe("block-level widgets in paragraphs (malformed markdown)", () => { | ||
| // Content authors sometimes forget to separate a block-level widget | ||
| // reference with blank lines (\n\n). The single \n before "[[☃ radio | ||
| // 1]]" here causes the radio widget to parse as an inline child of the | ||
| // paragraph. The renderer must split it out so the block widget is not | ||
| // nested inside a <p> (which would be invalid HTML). | ||
| const malformedContent = | ||
| "**Which picture shows how to measure the pink square?** \n" + | ||
| "[[☃ radio 1]]\n" + | ||
| "**The pink square is** [[☃ input-number 2]] ** blue squares tall.**"; | ||
|
|
||
| const malformedQuestion: PerseusRenderer = { | ||
| content: malformedContent, | ||
| images: {}, | ||
| widgets: { | ||
| "radio 1": generateRadioWidget(), | ||
| "input-number 2": generateInputNumberWidget(), | ||
| }, | ||
| }; |
Member
There was a problem hiding this comment.
This case isn't really malformed markdown. Paragraphs in markdown are allowed to contain line breaks. describe("block-level widgets in paragraphs" seems like an adequate description.
Comment on lines
+119
to
+121
| export const SPECIAL_WIDGET_TYPES: ReadonlySet<string> = new Set([ | ||
| "explanation", | ||
| ]); |
Member
There was a problem hiding this comment.
We need a better term for this than "special".
Since I suspect we don't want any more "special" widgets in the future, I propose removing the abstraction and just talking about explanation widgets as the special case.
const isExplanationWidgetNode = (node: any): boolean =>
node?.type === "widget" && node.widgetType === "explanation";
export const contentHasExplanationWidget = (node: any): boolean =>
Array.isArray(node.content) && node.content.some(isExplanationWidgetNode);| Array.isArray(node.content) && node.content.some(isSpecialWidgetNode); | ||
|
|
||
| const isWhitespaceOnlyTextNode = (node: any): boolean => | ||
| node?.type === "text" && !/\S/.test(node.content); |
Member
There was a problem hiding this comment.
The regex match is equivalent to
Suggested change
| node?.type === "text" && !/\S/.test(node.content); | |
| node?.type === "text" && node.content.trim() === "" |
which is probably more recognizable.
Comment on lines
+170
to
+188
| widgetBlock: { | ||
| // Process block-level widgets before paragraphs. | ||
| order: SimpleMarkdown.defaultRules.paragraph.order - 0.5, | ||
| // Match to the widget rule, but at the block level. | ||
| match: SimpleMarkdown.blockRegex(rWidgetRule), | ||
| // Type this as a "widget" so that the renderer will use | ||
| // the same rendering logic as inline widgets. | ||
| parse: (capture: any, parse: any, state: any): any => ({ | ||
| type: "widget", | ||
| id: capture[1], | ||
| widgetType: capture[2], | ||
| }), | ||
| react: (node, output, state) => { | ||
| // The actual output is handled in the renderer, where | ||
| // we know the current widget props/state. This is | ||
| // just a stub for testing. | ||
| return <em key={state.key}>{`[Widget: ${node.id}]`}</em>; | ||
| }, | ||
| }, |
Member
There was a problem hiding this comment.
Since we have splitBlockWidgetsFromParagraphs, can this widgetBlock rule be removed now?
| type ContentNodes = {blockNodes: Array<any>; inlineNodes: Array<any>}; | ||
| // Break the paragraph node into as many nodes as needed so that any | ||
| // block-level widgets are in their own node. | ||
| const {blockNodes, inlineNodes} = node.content.reduce( |
Member
There was a problem hiding this comment.
I think a for loop would be easier to read than reduce here — especially since we're mutating the blockNodes and inlineNodes arrays anyway.
const blockNodes = [];
const inlineNodes = [];
for (const childNode of node.content) {
// ...
}| return mergeInlineNodes(inlineNodes, blockNodes, node); | ||
| }); | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
These new functions should have tests.
| inlineNodes: Array<any>, | ||
| blockNodes: Array<any>, | ||
| referenceNode: any, | ||
| ): any | undefined => { |
Member
There was a problem hiding this comment.
Suggested change
| ): any | undefined => { | |
| ): any => { |
| blockNodes.push({...referenceNode, content: trimmedContentNodes}); | ||
| } | ||
| return blockNodes; | ||
| }; |
Member
There was a problem hiding this comment.
It feels weird that this function pushes its output into blockNodes — mutating its input in addition to returning the result. That makes the calling code hard to understand because it's not clear when looking at the callsite that blockNodes is going to be mutated.
I might inline mergeInlineNodes; then the first callsite would change to:
contentNodes.blockNodes.push({
...node,
content: trimEdgeWhitespaceNodes(inlineNodes),
});
Comment on lines
+18
to
+19
| return this.props.translationIndex !== undefined && | ||
| this.props.translationIndex !== null ? ( |
Member
There was a problem hiding this comment.
Suggested change
| return this.props.translationIndex !== undefined && | |
| this.props.translationIndex !== null ? ( | |
| return this.props.translationIndex != null ? ( |
We could also avoid the long ternary with an early return:
if (this.props.translationIndex == null) {
return this.props.children;
}
// For perseus-article just-in-place-translation (jipt), we need
// to attach some metadata to top-level QuestionParagraphs:
return (
<div ...>
{this.props.children}
</div>
);
mark-fitzgerald
commented
Jul 8, 2026
|
|
||
| const rWidgetParts = new RegExp(rWidgetRule.source + "$"); | ||
| const snowman = "\u2603"; | ||
|
|
Contributor
Author
There was a problem hiding this comment.
NOTE: I have unit tests coming for the changes added to this file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
The markdown renderer previously wrapped content in nested
<div class="paragraph">elements, producing non-semantic HTML that made styling difficult to scope and some bugs difficult to fix. This PR removes the outer<div class="paragraph">element and overrides theparagraphrendering rule to emit a real<p>element. It also adds awidgetBlockrendering rule so that block-level widgets are parsed at the block level instead of inside "paragraphs", and splits out block widgets that end up inside a paragraph when content is missing the blank lines that normally separate them. Inline widgets (definition, expression, numeric-input, etc.) still render inside the paragraph, while "special" inline widgets likeexplanationare rendered without a<p>wrapper. The result is a simpler, semantically correct DOM that's easier to style in a focused, scoped way.Issue: LEMS-4282
Test plan:
pnpm storybook) and view a few renderer stories that mix text, inline widgets (numeric-input, expression), and block widgets (radio).<p>elements, block-level widgets are siblings of paragraphs (not nested inside<p>), andexplanationwidgets render without a<p>wrapper.