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

Enable using Prism for syntax highlighting #735

Merged
merged 7 commits into from
Jun 9, 2018

Conversation

endiliey
Copy link
Contributor

@endiliey endiliey commented Jun 7, 2018

Motivation

Resolves #438

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Production Test
https://deploy-preview-735--docusaurus-preview.netlify.com/docs/en/next/doc-markdown#using-prism-as-additional-syntax-highlighter

Local Test

cd website
yarn start

Go to http://localhost:3000/docs/en/next/doc-markdown & scroll down

//siteConfig.js
usePrism: ['js', 'jsx'],
highlight: {
  theme: 'atom-one-dark',
}

atom-one-dark

//siteConfig.js
usePrism: ['js', 'jsx'],
highlight: {
  theme: 'default',
}

default

//siteConfig.js
usePrism: ['js', 'jsx'],
highlight: {
  theme: 'tomorrow-night-blue',
}

tomorrow-night-blue

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 7, 2018

Deploy preview for docusaurus-preview ready!

Built with commit 0b518ed

https://deploy-preview-735--docusaurus-preview.netlify.com

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

💯

I think package-lock.json will need to be updated for this as well, in addition to you yarn.lock?

@iRoachie would this work for you?

@JoelMarcey JoelMarcey requested a review from yangshun June 7, 2018 15:50
@iRoachie
Copy link
Contributor

iRoachie commented Jun 7, 2018

Looks GTM, is possible to have a usePrism: true to just use it for all languages instead of highlight?

@endiliey
Copy link
Contributor Author

endiliey commented Jun 7, 2018

That is doable. But it still fall back to hljs if the language is not supported by prism.

Additionally, I think user can override the prismJS css we use on their custom.css

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

I am good with this.

I would like a second approve from @yangshun on this, if possible.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Requesting changes just for discussion. Is it possible to lazy load the CSS only when the user sets usePrism? I don't think it's good to add CSS payload to all Docusaurus users when only a fraction of them will use Prism styling. I made a similar comment for the scroll to top button script but on hindsight should have also asked for it for the CSS.

return hljs.highlight(lang, str).value;
if (
siteConfig.usePrism === true ||
(siteConfig.usePrism && siteConfig.usePrism.indexOf(lang) !== -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use siteConfig.usePrism.length > 0 to be safer.

@@ -1871,3 +1871,134 @@ footer .social {
flex-shrink: 0;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This increases the CSS payload to all Docusaurus users, even though not all of them will use it. Is it possible to lazy load the CSS if usePrism is true?

@endiliey
Copy link
Contributor Author

endiliey commented Jun 8, 2018

@yangshun

Actually this main.css is a slightly modified version. I hacked it a little bit (like removing background) because if I don't, it can conflict with hljs theme.

Before:
cdn

After:
atom-one-dark

Is this okay for you ?

// Head.js
{this.props.config.usePrism && (
  <link
    rel="stylesheet"
    href={this.props.config.baseUrl + 'css/prism.css'}
  />
)}

I would love to use CDN but I've mentioned the problem above. Should I upload it somewhere & upload it to CDN ?
The link will look like this https://cdn.jsdelivr.net/gh/facebook/docusaurus@1.1.5/lib/static/css/main.css

@JoelMarcey
Copy link
Contributor

Leaving to @yangshun to approve.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Making it in a separate CSS file helps. Thanks @endiliey!

@yangshun yangshun changed the title Enable user to use prism.js as syntax highlighter Enable using Prism for syntax highlighting Jun 9, 2018
@yangshun yangshun merged commit c8bc00a into facebook:master Jun 9, 2018
@endiliey endiliey deleted the syntax branch July 4, 2018 17:30
@iRoachie
Copy link
Contributor

Finally got to try this out guys! Looks great. Encountered some small ux problems with the missing languages -- logged them in this ticket #1076

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

Successfully merging this pull request may close these issues.

None yet

6 participants