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 syntax highlighting (#309) #407

Merged
merged 4 commits into from
Nov 12, 2016
Merged

Enable syntax highlighting (#309) #407

merged 4 commits into from
Nov 12, 2016

Conversation

rnewton
Copy link
Contributor

@rnewton rnewton commented Oct 19, 2016

I decided to take a swing at adding syntax highlighting for the markdown preview. It uses highlight.js and the "hybrid" theme that seems to work well for both light and dark modes. Everything passes eslint and I think I followed the style guides, but let me know if you want any tweaks.

screenshot from 2016-10-18 21-10-45
screenshot from 2016-10-18 21-11-02

highlight: function( code ) {
return highlight.highlightAuto( code ).value;
}
} );
Copy link
Member

Choose a reason for hiding this comment

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

might start out by pulling this function into the module scope instead of creating a new closure on each call. could also use fat-arrow syntax for functions.

const highlighter = code => highlight.highlightAuto( code ).value;



const markdownHTML = marked( content, { highlight: highlighter } );

@dmsnell
Copy link
Member

dmsnell commented Oct 19, 2016

Thanks for the contribution @rnewton! I'll try and take a deeper look into this in the next day or two.

@rnewton
Copy link
Contributor Author

rnewton commented Oct 19, 2016

Good call. Thanks!

@@ -55,6 +55,7 @@
"cookie": "0.2.3",
"create-hash": "1.1.2",
"draft-js": "0.9.1",
"highlight.js": "^9.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

Let's pin this to its specific version. "Compatible" updates too often aren't.

@dmsnell
Copy link
Member

dmsnell commented Oct 19, 2016

@rnewton are there better choices of style for the dark theme vs. the light theme? I appreciate the balance you have picked, but I want to make sure we don't unnecessarily tie ourselves to one when we could switch between the two if that were better.

@rnewton
Copy link
Contributor Author

rnewton commented Oct 19, 2016

I'm definitely open to suggestions on the theme(s) to pick. highlight.js includes a bunch. I'm basically trying to avoid picking things that are divisive. Some people like monokai and some people hate it.

I'll admit that I didn't investigate light/dark as separate themes, but "atom one light/dark" might be a good fit.

@dmsnell
Copy link
Member

dmsnell commented Oct 19, 2016

@rnewton I noticed an opportunity to do the rendering in a web worker. probably this isn't the greatest fit at this time, but it would be nice to have a quick assessment if that's a good path to explore

@rnewton
Copy link
Contributor Author

rnewton commented Oct 19, 2016

I'm actually quite fond of solarized light/dark as an option instead. A little easier to read than the yellow on the light theme of hybrid.

screenshot from 2016-10-18 21-41-34
screenshot from 2016-10-18 21-41-45

@rnewton
Copy link
Contributor Author

rnewton commented Oct 19, 2016

Having never worked with web workers, I hesitate to jump on that right now...

@dmsnell
Copy link
Member

dmsnell commented Oct 19, 2016

@rnewton how hard would it be to swap styles at runtime?

@dmsnell
Copy link
Member

dmsnell commented Nov 5, 2016

@roundhill @drw158 and @rodrigoi this seems like a good future to put out there for 1.1.0 maybe? I think it's ready to ship, what do you all think?

@roundhill
Copy link
Contributor

@dmsnell sure, let's :shipit:

@dmsnell dmsnell merged commit a4ae064 into Automattic:master Nov 12, 2016
@davewhitley davewhitley mentioned this pull request Mar 29, 2017
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.

3 participants