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

DevDocs: add syntax highlighting #10359

Merged
merged 3 commits into from
Jan 10, 2017
Merged

DevDocs: add syntax highlighting #10359

merged 3 commits into from
Jan 10, 2017

Conversation

spen
Copy link
Contributor

@spen spen commented Jan 2, 2017

This PR adds syntax highlighting to the code blocks shown in DevDocs.
Currently code blocks are wrapped in <pre> and <code> tags via the marked package but the content itself is not processed for syntax.
@alisterscott pointed this out in a recent PR of mine (#10329)

This is not a perfect solution however... I've been working with the EmptyContent DevDoc page while developing and noticed that some .jsx syntax is not parsed properly.

emptycontent-syntax

In this PR marked is configured to use prismjs to handle syntax highlighting.
I've added some styling that is fairly close to GitHubs (at least in terms of colors).

So far I've tried a similar approach with highlight.js too, though this is also lacking in perfect .jsx support.

There is certainly an improvement but I wouldn't expect a merge right away until other options are explored.

I thought I'd get this up early in the hopes of starting a conversation - hopefully somebody out there has more experience than I do with highlighting packages :)

Testing

Test various syntax types, checking that A) they are highlighted and B) they are highlighted correctly (allowing for caveats mentioned in the discussion):

Notes: the only other syntax in use in code-blocks is 'text' (from a project-wide search for all instances of ``` ).

@alisterscott alisterscott added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Question labels Jan 3, 2017
@dmsnell
Copy link
Member

dmsnell commented Jan 5, 2017

This is great! thanks @spen!

the code looks fine to me but I'd be interested in knowing why you chose prism over highlight and other options if you also looked into them

@spen
Copy link
Contributor Author

spen commented Jan 5, 2017

So I only really looked in to Prism and Highlight.js as they seemed to surface the most.
I was really hoping to find and use the exact tool that github uses to be able to just use their OSS 'theme', matching any code-blocks one to one between GitHub & Calypso's DevDocs but from what I can gather it's a ruby gem (called prettylights I think) and it doesn't seem to be open source ( there's been discussion around this here: github/pages-gem#160 ).
I'm going to look in to one or two more tonight, including Atoms 'highlights' - being that it's a GH product I'd hazard a guess that it's fairly close to the output on GitHub.com.
After that I'll write something a bit more detailed... until then though it's mostly down to jsx syntax support (but neither Highlight.js or Prism seem to have it properly dialed in 100% 😕).

@spen
Copy link
Contributor Author

spen commented Jan 8, 2017

I've put together some notes on some of the open-source syntax highlighters currently available.
Feel free to chime in if there are any other good candidates that I've missed and I'll add them to this document 🙂

Requirements

Here're a few points that I think are important to keep in mind when considering one of the projects:

  • Should not be applied retrospectively to existing DOM elements (no relying on document.xx etc.) rather it should be applied to a string.
  • Compatible with marked since that's already in use.
  • Good support for .jsx & .es6 syntax - This should be a priority in Calypso.
    • Including JS inside of JSX inside of JS etc.
  • General OSS points - is it actively maintained, is it popular, well documented etc.

notes ...

Highlight.js

This seems popular and active.

Unfortunately it seems to struggle somewhat with JSX syntax - particularly JS within JSX.
Take a look at this render (Using v 9.1 - see notes):
screen shot 2017-01-08 at 13 00 29
And it's output markup:
screen shot 2017-01-08 at 13 01 15
Notice that syntax in { things.map( ... is not picked up on, rather left as one 'token'.

  • JSX support reportedly broken after v9.1 reference
  • Dan Abramov reports a JSX bug (here) and mentions that he has switched to prism (for it's better jsx support). Read on and you'll find that this issue was reopened in April 2016 and has been referenced a number of times since.

Prism

This also seems popular and active - I've seen a few people mention that they've made the switch from highlight.js to this package based on the .jsx support (see above).

Prism is extendable with plug-ins - which could prove useful if any Calypso-specific behaviour is needed.

There's a tool for testing Prism's highlighting here.
If you use the above code example then you'll notice that there's still the slight issue with jsx props
prism-props

For the initial commit here in this PR I chose to use Prism because of Dan Abramov the slight issues with the props seems more reasonable than losing support of the JS within JSX.

highlights

This highlighter is used in Atom's Markdown Preview.
Being that Atom is a GitHub product I would expect its syntax support to be inline with that of github.com's.
github.com seems to use a ruby gem named 'prettylights' which isn't open source, something to do with it holding a GPLv3 license (github/pages-gem#160 (comment)).

From what I can see, the intent is that this reads a file and outputs the tokenized contents.
Following the logic through, it looks like it uses the tokenizeLines method of this repo/file.

Negatives

  • Seems to only apply to file contents
  • The project seems very light on documentation
  • It's also written in coffeescript 😒

Syntaxhighlighter

I feel like I recently saw a block of code on wordpress.org that wasn't highlighted and assumed there was just no highlighting in place but when I went looking for an example I stumbled across this update that includes a highlighted block.

On inspection it turns out that this uses syntaxhighlighter, applied on the client side to the DOM nodes. Quickly tested by just loading the page with JS turned off.

The (brevityified) markup for the first line looks like this:

<!-- container -->
<div id="highlighter_696439" class="syntaxhighlighter  php">
<!-- omitting line numbers... -->
<code class="php plain">wp_nav_menu( </code>
<code class="php keyword">array</code>
<code class="php plain">(</code>
<!-- on so on -->

This approach is different to that of the other tools above in that instead of classing the containing <code> block with the language name and wrapping each keyword, punctuation etc. in span tags, each token is wrapped in a code block with class="{ language } {token}".

I've explored syntaxhighlighter a little, looked at it's source and docs and it doesn't seem to apply to strings (I may have missed something though) and so unfortunately I'd rule that out for Calypso.

I've not gone very deep with this package - since it's applied retrospectively to the DOM so I don't know how its JSX support is.


Ultimately it'd be awesome to have something that can be used universally, whether thats one syntax processor that can be used all over Automattic products... or at the very least a standard output from, say, a node syntax-processor and a PHP syntax-processor so that 'theme' css can be applied in all necessary places - JSX syntax support on wordpress.org should be identical to that on Calypso's DevDocs as it should be for code-blocks in Calypso's Reader.
Different themes could of cause be used to style these areas differently but the eventual markup should be reliably standard.

@dmsnell
Copy link
Member

dmsnell commented Jan 8, 2017

Thanks @spen - this is really a valuable assessment of the options. It seems very reasonable why you chose prism.

I support this PR 😄

background: #b3d4fc;
}

.token.comment,
Copy link
Contributor Author

@spen spen Jan 8, 2017

Choose a reason for hiding this comment

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

I believe there is a plug-in that allows for a custom class, which might be worth adding to better namespace these styles. I'll add that to #10359 (comment) .

@alisterscott
Copy link
Contributor

@spen: thanks for this PR. Can you please let me know when you consider it ready for testing and I can do some testing on it.

@spen
Copy link
Contributor Author

spen commented Jan 9, 2017

Sure thing! I've added some test instructions to the pull description so it could be tested now but I've not tested all of these syntaxes myself (I'm working on another project right now so can't test these myself until a little later) so you may want to wait until then.

Though I think it's ready to test I don't think it's necessarily ready to merge as I think the CSS could be improved, using variables etc.

@spen
Copy link
Contributor Author

spen commented Jan 10, 2017

It seems that ```es6 and ```json are not currently processed.
I'll aim to fix this today - after that this should be good to test :)

@spen
Copy link
Contributor Author

spen commented Jan 10, 2017

Ok, this should be good to test now (see the test instructions in the desc).

note:

I've hardcoded an alias of javascript to es6 so that ```es6 blocks are processed and highlighted correctly but I've also opened a pull upstream PrismJS/prism#1079 - if/when that makes it's way to a release I'll be able to remove the alias :)

Copy link
Contributor

@alisterscott alisterscott left a comment

Choose a reason for hiding this comment

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

This looks great and tests well - I will merge it now

@alisterscott alisterscott added [Status] Ready to Merge DevDocs and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 10, 2017
@alisterscott alisterscott merged commit 8204c7f into Automattic:master Jan 10, 2017
@alisterscott
Copy link
Contributor

Deployed and looking great on wpcalypso: eg: https://wpcalypso.wordpress.com/devdocs/client/components/empty-content/README.md

@spen spen deleted the update/add-code-block-syntax-highlighting-to-dev-docs branch January 10, 2017 06:17
@spen
Copy link
Contributor Author

spen commented Jan 10, 2017

Ahh awesome! Thanks @alisterscott !
Really good to see it looking pretty live!

I think I'll do some investigating in to how those string props might be better handled and, if I can figure it out, open an upstream PR :)

@@ -90,6 +90,7 @@
"phone": "git+https://github.com/Automattic/node-phone.git#1.0.8",
"photon": "2.0.0",
"postcss-cli": "2.5.1",
"prismjs": "^1.6.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

@spen let's try to pin all dependencies we add 👍 We've been burned by things like left-pad in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh sorry looks like this was already pinned elsewhere 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the nudge though @gwwar, always good to be thorough! :)

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

Successfully merging this pull request may close these issues.

5 participants