Skip to content

tools,doc: fix misrendering of consecutive JS blocks#40146

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-tools
Closed

tools,doc: fix misrendering of consecutive JS blocks#40146
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-tools

Conversation

@Trott
Copy link
Copy Markdown
Member

@Trott Trott commented Sep 18, 2021

Our markdown-to-html tool was assuming that any consecutive JS blocks
were ESM vs CJS alternatives, but that is not always the case, resulting
in both a confusing interface and invalid HTML.

Our markdown-to-html tool was assuming that any consecutive JS blocks
were ESM vs CJS alternatives, but that is not always the case, resulting
in both a confusing interface and invalid HTML.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Sep 18, 2021
@Trott
Copy link
Copy Markdown
Member Author

Trott commented Sep 18, 2021

There are three examples of this in the docs. Here is how a part of packages.html should be rendering (and how it renders with this change):

image

Here's how it looks currently:

image

The current situation results in a <pre> within another <pre> which is invalid HTML.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2021
@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented Sep 18, 2021

Weird, I thought I had taken this into consideration with line 227:

nextNode.lang !== node.lang) {

Maybe line 227 can be removed if it's actually not useful.

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Sep 18, 2021

Maybe line 227 can be removed if it's actually not useful.

I tried removing it but doing so ends up in garbled code examples elsewhere in the docs, so it seems like both statements handle different edge cases.

@targos targos added the fast-track PRs that do not need to wait for 72 hours to land. label Sep 19, 2021
@github-actions
Copy link
Copy Markdown
Contributor

Fast-track has been requested by @targos. Please 👍 to approve.

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 19, 2021
@github-actions github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 19, 2021
@github-actions
Copy link
Copy Markdown
Contributor

Landed in 0bafe6d...b0d5eec

@github-actions github-actions Bot closed this Sep 19, 2021
nodejs-github-bot pushed a commit that referenced this pull request Sep 19, 2021
Our markdown-to-html tool was assuming that any consecutive JS blocks
were ESM vs CJS alternatives, but that is not always the case, resulting
in both a confusing interface and invalid HTML.

PR-URL: #40146
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
Our markdown-to-html tool was assuming that any consecutive JS blocks
were ESM vs CJS alternatives, but that is not always the case, resulting
in both a confusing interface and invalid HTML.

PR-URL: #40146
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
Our markdown-to-html tool was assuming that any consecutive JS blocks
were ESM vs CJS alternatives, but that is not always the case, resulting
in both a confusing interface and invalid HTML.

PR-URL: #40146
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
1 task
@Trott Trott deleted the fix-tools branch September 25, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 72 hours to land. tools Issues and PRs related to the tools directory.

5 participants