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

Syntax highlighting does not work #19

Closed
jamelt opened this issue Dec 13, 2016 · 21 comments
Closed

Syntax highlighting does not work #19

jamelt opened this issue Dec 13, 2016 · 21 comments

Comments

@jamelt
Copy link

jamelt commented Dec 13, 2016

syntax highlighting will definitely not work out of the box for highlight.js. the code does not have the necessary bits in place

@NoahDragon
Copy link
Member

Could you please elaborate?

@jamelt
Copy link
Author

jamelt commented Dec 14, 2016

Take a look at the highlight.js stylesheets

...

.hljs {
  display: block;
  overflow-x: auto;
  padding: 0.5em;
  background: white;
  color: black;
}

.hljs-string,
.hljs-variable,
.hljs-template-variable,
.hljs-symbol,
.hljs-bullet,
.hljs-section,
.hljs-addition,
.hljs-attribute,
.hljs-link {
  color: #888;
}

...

All of the CSS classes have the prefix hljs-

The highlight.js file does not set the class prefix. It is impossible for any of these styles to work.

https:/hexojs/hexo-util/blob/master/lib/highlight.js#L8

hljs.configure({
  classPrefix: ''
});

Finally, the root class that applies the syntax background hljs is not appended to the code generation block.

https:/hexojs/hexo-util/blob/master/lib/highlight.js#L41

result += '<figure class="highlight' + (data.language ? ' ' + data.language : '') + '">';

@demurgos
Copy link
Contributor

demurgos commented Nov 3, 2017

Any news about this?
It seems there are two related pending PRs:

I don't get how to use syntax highlighting otherwise. I expect to simply inline one of the packaged styles.

npm install -S highlight.js

And then in my main.scss:

@import "~highlight.js/styles/default";

Due to the mismatch between the classes of the HTML and classes of the stylesheet, it does not work.
I am writing my own theme from scratch and there's no official documentation anywhere about how to handle syntax highlighting even if it is baked in the markdown renderer.

While looking at other themes, I cam across some solutions that seem a bit insane to me from a maintenance perspective.

// 4. Remove .hljs {...} rules
// 5. Remove hljs- of all CSS class name. E.g : .hljs-comment becomes .comment

Am I really expected to keep a hard copy of the packaged stylesheet (versioned in my repository) and manually edit it any time highlight.js is updated?

@ghost
Copy link

ghost commented Nov 8, 2017

As stupid as it is I'd say that's the preferable alternative given that there's not a whole lot to update in the stylesheets. Maybe you could use an older version of hljs from before the name scheme change with your mentioned method.

As you mentioned, #29 is in the works and should at least solve the naming issues; the failing unit test is however a bit over my level so hopefully @NoahDragon can figure that out.

@demurgos
Copy link
Contributor

demurgos commented Nov 8, 2017

Thanks for the reply.
To be honest, I feel that the problem is also partly due to the choice of highlight.js to only provide CSS stylesheet instead of a more advanced scss format. This effectively forces you to adhere to their naming scheme or manually edit your file. Overall, I believe that code highlighting is an area where Hexo could be improved.

@ghost
Copy link

ghost commented Nov 8, 2017

You could try out hexo-prism as an alternative if you'd like.

Overall there's a lot to improve about hexo, but that's kind of what I like about it: nothing is quite set in stone, unlike other static site generators I've tried :)

@demurgos
Copy link
Contributor

demurgos commented Nov 8, 2017

The problem is that I do not highlight my code myself. I use use the Markdown plugin which in turn uses this plugin. Of course I love the fact that Hexo is fully configurable, but it should still provide a good out-of-the-box experience.
I started with Hexo a few months ago and just used an existing theme: it was great. Now I decided to dig a bit deeper and do my own theme/try some advanced features: let's just say that it was the occasion to visit the issue boards 😄. Still, I am overall very happy with the maintainers: they seem to care and are very reactive. I still need to get familiar with the code base but I'll try to provide some help when possible.

When I come to think about it, I find it a bit odd that code highlighting does not have its own repo. The other main features of this package seem to be about input sanitization and process spawning so separating code highlighting shouldn't have a big impact. After all, it's mostly a façade for highlight.js anyway.

@NoahDragon
Copy link
Member

@Krakob Thanks for poking me about this. I will take a look at #29 now before my procrastination arises.

@alxbl
Copy link
Contributor

alxbl commented Dec 11, 2017

Take a look at #30. It adds a configuration setting to enable CSS support.

I have some changes pending review that should make it possible to use highlight.js themes.
(I hadn't noticed that #29 existed until I submitted #30)

@demurgos
Copy link
Contributor

demurgos commented Dec 11, 2017

After thinking about it a bit more, I believe that part of the issue may caused by highlight.js itself. They offer an API to let you configure the HTML class but nothing for CSS. Since they allow you to choose your HTML class, they should offer sass equivalents for CSS. This way you could have custom classes and matching stylesheets.
Unfortunately, they only support CSS because they did not want to favor any CSS preprocessor. I believe that a better long-term solution would be a repo automatically importing their css files, converting them to sass and publishing them so you can build your own stylesheets.

@alxbl
Copy link
Contributor

alxbl commented Dec 11, 2017

@demurgos The changes I mentioned should make it possible to drop-in any highlight.js stylesheet and have the theme work properly, since all the themes are now hljs- prefixed.

Ideally the hljs defaults should also be the defaults used by hexo-util, but unfortunately making the changes the new default breaks existing themes that use the old non-prefixed themes. I'm not a core maintainer so I didn't want to do that unless the maintainers believe it's the way ahead.

For custom themes, it should be possible to have a high-level stylesheet in your preprocessor language of choice with a palette sheet that declares the variables. Your theme could then use that style and you'd get the best of both words. No need to transform the existing styles.

One step further would be to have some sort of built-in support (_config.yml?) for customizing the palette, but given the options above, I don't think that's necessary.

@curbengh
Copy link
Contributor

Anyone can give a brief description of the issue?
My understanding is that hexo doesn't append hljs- to class name, like so.
So is this a feature request have an option to prepend hljs-?

@SukkaW
Copy link
Member

SukkaW commented Sep 14, 2019

@curbengh

In short, newer version of highlight.js use css class name with hljs- prepend, while hexo built in highlight.js still using class name highlight.
Thus, the solution is quite easy: Download highlight.js css, replace all the .hljs- with .highlight (with a space affixed). Using old version of highlight.js themes will do as well.

IMHO, since there are already many developed hexo-theme are using .highlight in their css, change this behavior in hexo might cause Breaking Changes and caused many hexo-theme to break.
Also, hexo built in highlight.js is very slow, deprecate it is already on the roadmap.

@tomap
Copy link
Contributor

tomap commented Oct 5, 2019

@SukkaW @curbengh the solution here is:

  1. include the adapted version of the css (as @SukkaW suggests, from here https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.15.10/styles/default.min.css)
    Where do we include that? in the default Hexo themes?
  2. deprecate highlight.js, and replace is with what? https://prismjs.com/
    what would be the place for that?

@SukkaW
Copy link
Member

SukkaW commented Oct 6, 2019

@tomap

Adapt highlight css to fit the old version of highlight.js should be done in theme, not in hexo.

For hexo, we should just deprecate highlight.js since @tommy351 said it is better to use browser side highlight (hexojs/hexo#1036 (comment)). Only if prismjs has better performance should we add built-in support for it.

@tomap
Copy link
Contributor

tomap commented Oct 6, 2019

@SukkaW I have a site without JS and intend to keep it that way. So I enjoy very much server side rendering :)
I would like to have the option!
For the .css file, maybe there should by a documentation (in site) about the CSS to include, maybe referencing a .css file in this repo that we would maintain, so that theme owner can use it as a starting point

@SukkaW
Copy link
Member

SukkaW commented Oct 6, 2019

@curbengh
Copy link
Contributor

curbengh commented Oct 8, 2019

2. deprecate highlight.js, and replace is with what? https://prismjs.com/
   what would be the place for that?

Atom's highlights is another candidate. The benchmark seems good. There are also many Atom-compatible themes that theme dev can adapt. In regards to #39, it seems to support support jsx.

But the overall supported languages are still much less than highlight.js and prism. Most of the lang submodules haven't been updated (to their latest respective commit) for >3 years. Looking through the js highlight module, it seems some ES6+ syntax is not highlighted. I assume other langs are also not updated frequently as well.


Anyway, why not support all? highlight.js (old & new syntax), atom highlights and prism in hexo-util would be nice. This way it doesn't break existing theme, while at the same time theme can choose whichever to support. If theme prefers client-side, can always disable server-side (currently it can be disabled).

to illustrate:

highlight:
  enable: false|true #existing option
  engine: highlight|atom|prism

I concur with @tomap with having server-side rendering, personally I'm willing to sacrifice generation time to minimize the amount of client-side js.

@SukkaW
Copy link
Member

SukkaW commented Oct 8, 2019

@tomap @curbengh

I suggest add support for prism first, and I love the highlight.engine idea. There is already a hexo-prism-plugin made by @ele828, and it is easy for us to learn something from it.

@curbengh
Copy link
Contributor

curbengh commented Oct 9, 2019

@ #19 (comment)

All of the CSS classes have the prefix hljs-

The highlight.js file does not set the class prefix. It is impossible for any of these styles to work.

Finally, the root class that applies the syntax background hljs is not appended to the code generation block.

This has been fixed in hexojs/hexo#2901 + #30

To prepend hljs-

_config.yml
highlight:
+  hljs: true

@curbengh
Copy link
Contributor

@jamelt

Feel free to re-open if the config doesn't work.

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

No branches or pull requests

7 participants