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

docs(markdown): parse :::note's text as MD elements #32510

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Sep 9, 2024

Two ways of fixing it, either:

a) Pass flattenText: falase to wrapText's options, but it seems to come from high up and overriding it only for :::note seems not great.
b) Parse the :::note's children as real MarkdownNotes and render them as real notes

I did b) in this case, which seems more reasonable.

Fixes #32505
Preview of the rolled docs: microsoft/playwright.dev#1503

This comment has been minimized.

@mxschmitt mxschmitt force-pushed the fix-markdown-notes branch 2 times, most recently from 5acc78d to cdd5bd5 Compare September 9, 2024 08:37

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@mxschmitt mxschmitt force-pushed the fix-markdown-notes branch 4 times, most recently from 85b9b49 to 0d97fc4 Compare September 11, 2024 12:17

This comment has been minimized.

utils/markdown.js Outdated Show resolved Hide resolved
@@ -802,12 +802,12 @@ function generateSourceCodeComment(spec) {
node.codeLang = parseCodeLang(node.codeLang).highlighter;
if (node.type === 'note') {
// @ts-ignore
node.type = 'text';
node.text = '**NOTE** ' + node.text;
node.type = 'text'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no semicolons?

Copy link
Member Author

Choose a reason for hiding this comment

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

its JS, we don't lint JS.

utils/doclint/documentation.js Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

}
innerRenderMdNode(indent, textNode, lastNode, result, options);
} else {
result.push(`${indent}:::${node.noteType}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
result.push(`${indent}:::${node.noteType}`);
result.push(options.notePrefix || `${indent}:::${node.noteType}`);

const children = node.children ?? [];
for (let i = 0; i < children.length; i++)
innerRenderMdNode(indent, children[i], lastNode, result, options);
result.push(`${indent}:::`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
result.push(`${indent}:::`);
result.push(options.noteSuffix || `${indent}:::`);

* renderCodeBlockTitlesInHeader?: boolean
* flattenText?: boolean,
* renderCodeBlockTitlesInHeader?: boolean,
* renderNoteAsText?: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend notePrefix?: string, noteSuffix?: string. This way, you can render code blocks or lists inside notes, even for typescript comments.

Comment on lines +353 to +354
for (let i = 0; i < children.length; i++)
innerRenderMdNode(indent, children[i], lastNode, result, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (let i = 0; i < children.length; i++)
innerRenderMdNode(indent, children[i], lastNode, result, options);
for (const child of children) {
innerRenderMdNode(indent, child, lastNode, result, options);
lastNode = child;
}

Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [webkit-codegen-mode-trace] › library/inspector/cli-codegen-2.spec.ts:408:7 › cli codegen › click should emit events in order

1 flaky ⚠️ [chromium-library] › library/capabilities.spec.ts:141:3 › should not crash on showDirectoryPicker

36325 passed, 743 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs]: NOTE in the Introduction / Clock page
2 participants