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

Improve syntax highlighting #32

Closed
wants to merge 1 commit into from
Closed

Improve syntax highlighting #32

wants to merge 1 commit into from

Conversation

mattdesl
Copy link
Collaborator

@mattdesl mattdesl commented Jul 7, 2015

A minor irk for me is that the syntax highlighting is not accurate to GH, and it also tries to highlight non-code blocks (plain text with <pre>).

This improves it slightly, see the screen shots attached. However it might have other side-effects, so I will sit on it for a while and test.

I think ideally Pygments would be used to highlight exactly as it appears on GH, but this may involve doing it in server.js and then sending the HTML string back to client. The upside is that it will not have a flash of unstyled code.

Before fix:

screen shot 2015-07-07 at 11 21 27 am

After fix:

screen shot 2015-07-07 at 11 21 39 am

@maxkueng
Copy link
Collaborator

maxkueng commented Jul 9, 2015

Looks much better (screenshots). Have you noticed any side effects yet?

Regarding Pygments: Is this what GitHub uses? It depends on Python. Not sure if this could be a problem. I think most people who have Node.js and/or npm installed will probably also have Python as node-gyp requires it. There's a pygmentize-bundled package on npm which bundles Pygments so users don't have to manually install Pygments themselves. I have never used it though.

Pygments would be used to highlight exactly as it appears on GH, but this may involve doing it in server.js and then sending the HTML string back to client.

Yes, because Pygments has to be started as a child process. I don't see a problem with having this code in the browser process (server.js) and sending it over to the renderer over IPC. It might even be the cleaner solution.

@mattdesl
Copy link
Collaborator Author

mattdesl commented Jul 9, 2015

No issues with it yet.

@maxkueng
Copy link
Collaborator

maxkueng commented Jul 9, 2015

Feel confident to merge it? I can create a "spin-off" issue for the Pygments stuff.

@yoshuawuyts
Copy link
Owner

Merged in 7a0523c, released as v1.7.1

@yoshuawuyts yoshuawuyts deleted the fix/highlight branch July 11, 2015 00:44
@yoshuawuyts yoshuawuyts mentioned this pull request Jul 11, 2015
@maxkueng
Copy link
Collaborator

Merged in 7a0523c, released as v1.7.1

👍

@yoshuawuyts: Can you push tags? 1.7.1 tag isn't there yet.

@yoshuawuyts
Copy link
Owner

Odd, somehow my script didn't always push tags. Now it should though 😅 - https://github.com/yoshuawuyts/dotfiles/blob/master/bin/npv#L16 Thanks!

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