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

fix: MD046/code-block-style #2557

Merged
merged 2 commits into from
Oct 1, 2019

Conversation

nschonni
Copy link
Member

@nschonni nschonni commented Sep 8, 2019

Code block style of fenced to match nodejs style

@XhmikosR
Copy link
Contributor

XhmikosR commented Sep 9, 2019

@Trott please don't merge this!

@nschonni did you run npm start locally? This throws, or at least should throw, when a prism lang is specified that doesn't exist.

For example:

Failed to load prism syntax: console
Error: Cannot find module 'prismjs/components/prism-console.js'
Require stack:
- C:\Users\xmr\Desktop\nodejs.org\node_modules\metalsmith-prism\lib\index.js
- C:\Users\xmr\Desktop\nodejs.org\build.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:772:15)
    at Function.Module._load (internal/modules/cjs/loader.js:677:27)
    at Module.require (internal/modules/cjs/loader.js:830:19)
    at require (internal/modules/cjs/helpers.js:68:18)
    at requireLanguage (C:\Users\xmr\Desktop\nodejs.org\node_modules\metalsmith-prism\lib\index.js:42:11)
    at Object.<anonymous> (C:\Users\xmr\Desktop\nodejs.org\node_modules\metalsmith-prism\lib\index.js:86:11)
    at initialize.exports.each (C:\Users\xmr\Desktop\nodejs.org\node_modules\cheerio\lib\api\traversing.js:300:24)
    at C:\Users\xmr\Desktop\nodejs.org\node_modules\metalsmith-prism\lib\index.js:62:17
    at C:\Users\xmr\Desktop\nodejs.org\node_modules\lodash\lodash.js:4905:15
    at baseForOwn (C:\Users\xmr\Desktop\nodejs.org\node_modules\lodash\lodash.js:2990:24) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    'C:\\Users\\xmr\\Desktop\\nodejs.org\\node_modules\\metalsmith-prism\\lib\\index.js',
    'C:\\Users\\xmr\\Desktop\\nodejs.org\\build.js'
  ]
}

@XhmikosR
Copy link
Contributor

XhmikosR commented Sep 9, 2019

We really need #2539, although to be fair, in this case these are wrapped in try/catch so it doesn't throw. But at least we would see them in our console.

@nschonni nschonni force-pushed the fix--MD046/code-block-style branch from e06a92f to 6577dbb Compare September 9, 2019 04:34
@nschonni
Copy link
Member Author

nschonni commented Sep 9, 2019

OK, just removed the "console" ones with plain blocks

@XhmikosR
Copy link
Contributor

XhmikosR commented Sep 9, 2019

@Trott: do you know which library is used upstream for highlighting? It seems metalsmith-prism is archived, so it might be worth looking into switching to the same way it's done upstream in another PR.

@nschonni nschonni force-pushed the fix--MD046/code-block-style branch from 6577dbb to ceef027 Compare September 9, 2019 04:54
@Trott
Copy link
Member

Trott commented Sep 9, 2019

@Trott: do you know which library is used upstream for highlighting? It seems metalsmith-prism is archived, so it might be worth looking into switching to the same way it's done upstream in another PR.

Looking at https://github.com/nodejs/node/blob/6723169097759502da11c47389ffb3d650404417/tools/doc/html.js, I guess we're using https://github.com/remarkjs/remark-rehype.

@XhmikosR
Copy link
Contributor

This isn't ready IMO.

When we use fenced code blocks, we need to remove any indented code otherwise we'll end up with extra spaces in the snippets. It seems there are some cases this is missed like https://github.com/nodejs/nodejs.org/pull/2557/files#diff-74f34b44a0285dc0c6cd1f2eeed2e2b5

I didn't go through all of the changes, so I don't know if there are more.

@nschonni nschonni force-pushed the fix--MD046/code-block-style branch from ceef027 to 64a1981 Compare September 15, 2019 05:27
@XhmikosR
Copy link
Contributor

LGTM except for my comment above

@nschonni nschonni force-pushed the fix--MD046/code-block-style branch 2 times, most recently from aaeca57 to fa99d0a Compare September 21, 2019 04:22
@nschonni
Copy link
Member Author

@Trott I think all the major stuff is resolved now. The indentation stuff is as-is for the existing examples

@Trott
Copy link
Member

Trott commented Sep 25, 2019

@Trott I think all the major stuff is resolved now. The indentation stuff is as-is for the existing examples

This is a big change set. @nodejs/website Anyone up for trying to give this a review?

@XhmikosR
Copy link
Contributor

XhmikosR commented Sep 25, 2019

@Trott we should move forward with #2523 and #2524 so that we don't have to review the snippets manually and that we are consistent. I've left a comment on #2523.

As far as this PR goes, it's definitely an improvement. I'd only go through the snippets and fix the indentation while at it. Assuming we don't use any code block languages that prism doesn't support (like console) this should be OK. It also fixes a broken example in the pt_BR translation.

@nschonni
Copy link
Member Author

Easiest way to review it is with the "whitespace ignored" option set, then you just see the code fence lines and the example fix

@XhmikosR
Copy link
Contributor

Yeah, but in this case whitespace does matter. So, the only safe way is viewing the rich preview of each file which takes time though.

@XhmikosR
Copy link
Contributor

This looks good to me. There are 3 unresolved comments and then it can be merged IMO.

@XhmikosR
Copy link
Contributor

@nschonni can you just address the 2 indentation comments while you change the code snippets?

Otherwise someone with push rights could fix them and then just squash.

Code block style of fenced to match nodejs style
Multiple spaces after blockquote symbol
@nschonni nschonni force-pushed the fix--MD046/code-block-style branch from fa99d0a to 5b17d67 Compare September 30, 2019 22:22
@nschonni nschonni requested a review from Trott September 30, 2019 22:27
@Trott Trott merged commit 6b7f09a into nodejs:master Oct 1, 2019
@nschonni nschonni deleted the fix--MD046/code-block-style branch October 1, 2019 03:36
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.

4 participants