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

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

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
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
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
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
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
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 48 hours to land. label Sep 19, 2021
@github-actions
Copy link
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
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 48 hours to land. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants