added syntax highlighting for markdown codes #716

mtxr · 2017-11-11T16:10:34Z

Resolves #591 #246.

Added highlight.js lib for markdown syntax highlighting on details page.
Static file generate following this guide http://highlightjs.readthedocs.io/en/latest/building-testing.html
- Use command: node tools/build.js -t javascript typescript css xml bash shell
Added monokai (well know, highly recommend throught the web) theme as default theme for syntax highlighting

Hope you enjoy.

Haroenv · 2017-11-11T16:14:06Z

Deploy preview ready!

Built with commit 9693228

https://deploy-preview-716--yarnpkg.netlify.com

…il page

mtxr · 2017-11-11T18:41:25Z

Syntax highlight is now working for the details page.

Also, I applied the same theme for rougeHighligther and highlight.js so all markdowns will be using the same style.

Haroenv · 2017-11-12T21:25:18Z

Oh my awesome work! Thanks so much. I’ll review this tomorrow

Haroenv · 2017-11-12T21:27:34Z

js/src/lib/Details/Markdown.js

@@ -1,6 +1,7 @@
 import React from 'react';
 import marked from 'marked';
 import xss from 'xss';
+import hljs from 'highlight.js';


How much does this increase the bundle?

Where can I check this?

highlight.js is 20K without minification. Probably it will add +10K at least to the current website.

https://deploy-preview-716--yarnpkg.netlify.com/en/

I’m wondering if there could be a way to have this only in the bundle where needed 🤔

I think you're loading Highlight.js twice now - Once in _layouts/default.html and once here.

I also think that https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.12.0/highlight.min.js contains every language supported by Highlight.js. I went to the Highlight.js site (https://highlightjs.org/download/) and downloaded a build that only supports Markdown, and it's only 9.4 KB minified / 4.8 KB minified and gzipped. That might be the best thing to do here? I wonder if you could do that using the npm package. Otherwise, I think downloading a minimal Highlight.js and sticking it in the repo is fine.

I think we can start with supporting JS and CSS for now?

Haroenv · 2017-11-12T21:49:42Z

I think it introduced some errors in parsing inline html:

the bundle increased by about 800k, so I’m hoping we find a solution with less overhead. Can we limit the languages included and make sure it’s only loaded as needed (maybe with a dynamic import or something webpackey) ?

mtxr · 2017-11-12T21:59:15Z

Wow, that was unexpected!

Probably is because I had to switch the place of xss, I'll try something else.
About the dynamic import, I need to study a bit more, React is not my fw :(

Haroenv · 2017-11-12T22:06:32Z

Oh and I can’t say enough that I’m really grateful for you helping out!

Daniel15 · 2017-11-13T02:05:51Z

js/src/lib/Details/Markdown.js

@@ -1,6 +1,7 @@
 import React from 'react';
 import marked from 'marked';
 import xss from 'xss';
+import hljs from 'highlight.js';


I think you're loading Highlight.js twice now - Once in _layouts/default.html and once here.

I also think that https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.12.0/highlight.min.js contains every language supported by Highlight.js. I went to the Highlight.js site (https://highlightjs.org/download/) and downloaded a build that only supports Markdown, and it's only 9.4 KB minified / 4.8 KB minified and gzipped. That might be the best thing to do here? I wonder if you could do that using the npm package. Otherwise, I think downloading a minimal Highlight.js and sticking it in the repo is fine.

Daniel15 · 2017-11-13T02:06:43Z

_config.yml

@@ -53,15 +53,10 @@ gems:
 markdown: kramdown
 kramdown:
  input: GFM
-  syntax_highlighter: rouge
+  syntax_highlighter: none


I'd strongly recomment keeping rouge enabled. This is highlighting the code on the site as part of the build, rather than client-side. This means that JS is not required at all. We only really need the JS version for the package details page, everything else can do build-time highlighting. Disabling this means that every page that displays code needs to load the JS highlighter.

Daniel15 · 2017-11-13T02:08:48Z

Here's webpack's docs on "code splitting": https://webpack.js.org/guides/code-splitting/. It also talks about dynamic imports.

mtxr · 2017-11-13T11:54:57Z

Thanks! I will try to fix later today!

Haroenv · 2017-11-13T14:18:14Z

js/src/lib/Details/Markdown.js

-  return marked(source, { renderer, mangle: false, sanitize: true });
+  const html = marked(source, { renderer, mangle: false });
+  return xss(html, {
+    whiteList: Object.assign({}, xss.getDefaultWhiteList(), {


you can use spreading here if you want, it's supported by our babel config 😄

Thanks! just changed.

Haroenv · 2017-11-13T14:19:12Z

js/src/lib/Details/Markdown.js

@@ -1,7 +1,7 @@
 import React from 'react';
 import marked from 'marked';
 import xss from 'xss';
-import hljs from 'highlight.js';
+import hljs from '../../../../_js/highlight/build/highlight.pack';


It's probably simpler if we just include this file, WDYT @Daniel15 ?

I'll try to split the code later, but I didn't understand really why to do that

Haroenv · 2017-11-13T14:21:54Z

scripts/highlight.sh

 OLD_CWD=$(pwd)
 cd $ROOT_PATH/_js/highlight
 rm -rf build/ &> /dev/null
+npm install


you can use yarn here if it's run on Netlify, or we can track the built version, since this will probably almost never change 🤔

Haroenv · 2017-11-13T14:22:55Z

> ./scripts/highlight.sh
3:21:29 PM: module.js:471
    throw err;
    ^

Error: Cannot find module 'gear'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/opt/build/repo/_js/highlight/tools/build.js:65:32)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

mtxr · 2017-11-13T14:26:31Z

@Haroenv could you help me with the build?

To use a smaller version of highlight.js we need to build it from source, but i'm stuck with netlify

Haroenv · 2017-11-13T14:51:28Z

so I get it to work locally, still not fully sure why it doesn't work on Netlify. Can you see the build logs?

Haroenv · 2017-11-13T14:52:10Z

I'm wondering what causes laggy horizontal scroll on http://127.0.0.1:4000/lang/en/package/?redux for example

Haroenv · 2017-11-13T15:03:29Z

_js/highlight/.travis.yml

+  -
+  - BROWSER=1
+  - BROWSER=1 NOCOMPRESS=1
+script:


Maybe a better place for this might be scripts/highlight, WDYT?

you can hoist all the dependencies to devdep in the main one I think, or use yarn workspaces

I'll try to use the built version without submodules. Seems that netlify build process can't run git commands. Maybe I'm wrong

Can i see travis logs?

the logs are available if you click on the error normally

https://app.netlify.com/sites/yarnpkg/deploys/5a09b523a114770371f9c8fc for example

mtxr · 2017-11-13T15:14:58Z

Aaannndd it's alive!

Haroenv · 2017-11-13T15:21:14Z

This is awesome, build size (obviously) increased a little bit (page is now 1.2MB over 1MB earlier). Comment I made earlier about scrolling sideways not being super smooth is still valid, and (I'm not sure if it's still worth it now) some way to split the bundle out of the main common-hash.js may be a good idea too

mtxr · 2017-11-13T15:22:56Z

Ok! Just give me sometime to study a bit more and I can fix that.

mtxr · 2017-11-13T15:25:27Z

Scrolling seems to be ok here. Can you upload a gif please?

Haroenv · 2017-11-13T15:29:05Z

funny, scrolling wasn't laggy while recording, I guess it's okay then.

Haroenv · 2017-11-13T15:29:56Z

we have the static highlight.js file now (okay for me), but maybe for reference (you can just write this in this PR), how is it generated?

Haroenv

LGTM now, @Daniel15 can you also review this still?

mtxr · 2017-11-13T15:35:05Z

Sure.

Done!

Haroenv · 2017-11-15T12:27:06Z

Congrats on doing this, I'm really excited. We can see later what some ways of improving further can be 🔥

mtxr · 2017-11-15T13:48:54Z

Nice! Thanks!

Haroenv · 2017-11-15T17:30:37Z

@mtxr, what's your twitter handle? I accidentally mentioned someone else here: https://twitter.com/haroenv/status/930848700901089281

mtxr · 2017-11-15T22:17:19Z

I don't really use twitter ;(, I have an account just for development purposes.

You can mention with you want, it's @mtxr_dev, but if you prefer, you can refer my github profile ;)

Thanks!

added syntax highlighting for markdown codes

3031436

mtxr added 9 commits November 11, 2017 14:32

Added highlight to markdown renderer

9be463f

Markdown code change test

c21db1c

Logging marked errors

9005551

Using sanitize

69568ad

highlight styles

057589e

Removed unsed consoles

43d4725

Using monokai as highlight theme

7e125d9

Using monokai for all markdown code, syntax highlight working on deta…

57dc581

…il page

Surounded with pre code tags

fff0d9b

Haroenv reviewed Nov 12, 2017

View reviewed changes

Daniel15 reviewed Nov 13, 2017

View reviewed changes

mtxr added 3 commits November 13, 2017 11:15

Disable hljs and using rouge for builds

51fad1e

fied broken detail page

d57ea43

Added minified highlight.js with common languages

0de3e19

Haroenv reviewed Nov 13, 2017

View reviewed changes

Fixed build script

abc9705

Haroenv reviewed Nov 13, 2017

View reviewed changes

Fixed build script to use yarn

5225bba

mtxr added 2 commits November 13, 2017 12:38

Using spreading

d46f8d2

Fixed build script to use yarn

c1fa4b4

mtxr added 2 commits November 13, 2017 12:52

Removed submodule

e1e4718

Using hightlight statically

93f4e38

Haroenv reviewed Nov 13, 2017

View reviewed changes

mtxr added 2 commits November 13, 2017 13:07

Using built highlight.js

08e49b0

Static highlight.js

48bf36c

removed unnecessary changes

8d244ae

style(markdown): simplify css config

9693228

Haroenv approved these changes Nov 13, 2017

View reviewed changes

Haroenv merged commit 3cd9aad into yarnpkg:master Nov 15, 2017

added syntax highlighting for markdown codes #716

added syntax highlighting for markdown codes #716

Conversation

mtxr commented Nov 11, 2017 • edited Loading

Haroenv commented Nov 11, 2017 • edited Loading

mtxr commented Nov 11, 2017

Haroenv commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel15 Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haroenv commented Nov 12, 2017 • edited Loading

mtxr commented Nov 12, 2017

Haroenv commented Nov 12, 2017

Daniel15 Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Daniel15 Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel15 commented Nov 13, 2017

mtxr commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haroenv commented Nov 13, 2017

mtxr commented Nov 13, 2017

Haroenv commented Nov 13, 2017

Haroenv commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtxr commented Nov 13, 2017

Haroenv commented Nov 13, 2017

mtxr commented Nov 13, 2017

mtxr commented Nov 13, 2017

Haroenv commented Nov 13, 2017

Haroenv commented Nov 13, 2017

Haroenv left a comment

Choose a reason for hiding this comment

mtxr commented Nov 13, 2017

Haroenv commented Nov 15, 2017

mtxr commented Nov 15, 2017

Haroenv commented Nov 15, 2017 • edited Loading

mtxr commented Nov 15, 2017

mtxr commented Nov 11, 2017 •

edited

Loading

Haroenv commented Nov 11, 2017 •

edited

Loading

Daniel15 Nov 13, 2017 •

edited

Loading

Haroenv commented Nov 12, 2017 •

edited

Loading

Daniel15 Nov 13, 2017 •

edited

Loading

Daniel15 Nov 13, 2017 •

edited

Loading

Haroenv commented Nov 15, 2017 •

edited

Loading