Make consistency in output of "highlight()" for "wrap: false" (fix #111) #112
Conversation
|
This PR will be used to introduce |
lib/highlight.js
Outdated
| @@ -21,13 +21,15 @@ function highlightUtil(str, options = {}) { | |||
| hljs.configure({ classPrefix: useHljs ? 'hljs-' : ''}); | |||
|
|
|||
| const data = highlight(str, options); | |||
| const lang = options.lang || ''; // Maybe "data.language" should be included | |||
There was a problem hiding this comment.
My understanding is that data.language would default to plain value if no lang is provided. In current behavior wrap = true, plain does get appended to class name -> "highlight plain".
So, I agree with options.lang || data.language || '';.
There was a problem hiding this comment.
One thing I worry about is that the default behavior of Hexo with hljs: true will be changed.
Under the code options.lang || data.language || '',
when hljs:true is specified with neither lang option nor gutter option,
the output of highlight() will be <pre><code class="hljs plain">....
On the other hand, the current implementation says <pre><code class="hljs undefined">....
That is, a class name plain is not added (adding undefined is a tiny bug).
Under the code options.lang || '' (without data.language),
the default output of highlight() will be <pre><code class="hljs">....
That is, no class name is added.
So including data.language causes Hexo changing the default behavior with hljs: true, line_number: false.
Is it OK?
There was a problem hiding this comment.
the current implementation says
<pre><code class="hljs undefined">
which implementation are you referring to? this PR with options.lang || data.language || ''?
The main reason of having "highlight plain" is so that "hljs: false, wrap: false" can be compatible with current theme as much as possible (which uses the default "hljs: false, wrap: true". I just worry that some theme might use "plain" class.
So including data.language causes Hexo changing the default behavior with
hljs: true, line_number: false.
I don't foresee any breaking change for adding a new class (i.e. "plain"), even when it's not used in the newer syntax.
In summary, without data.language, "plain" class is removed when "hljs: false, wrap: false" which is a possible breaking change. Having an unused "plain" class in "hljs: true" is not a breaking change.
There was a problem hiding this comment.
@curbengh Thank you for explaining. I didn't understand enough.
Now I understand that data.language is necessary to keep compatibility with current themes and adding a class plain will not break current themes.
I'll update the code soon.
seaoak
force-pushed
the
feature/make_consistency_in_output_when_wrap_false
branch
from
73ce38f to
a8de9f5
Compare
|
I updated the code of this PR (reflect @curbengh 's comments and add two test cases). |
|
Since Definitely we'll have to mention in the docs about the compatibility (after |
|
I agree with giving the priority ( I will update this PR to arbitrate the conflict with them. |
seaoak
force-pushed
the
feature/make_consistency_in_output_when_wrap_false
branch
from
a8de9f5 to
980adfd
Compare
…xojs#111) This patch is fix the behavior of `highlight()` with a option `wrap: false`. If `hljs: false`, the output of `highlight()` should include a HTML class name `highlight`. It is consistent with the case of `hljs:true`. And also, even if `hljs: false`, the output of `highlight()` should include a PRE element and a CODE element. It is consistent with the case of `hljs:true`, too.
seaoak
force-pushed
the
feature/make_consistency_in_output_when_wrap_false
branch
from
980adfd to
62ab0f8
Compare
|
@curbengh I'm sorry it was an elementary mistake. |
Fix #111
This patch is fix the behavior of
highlight()with a optionwrap: false.If
hljs: false, the output ofhighlight()should include a HTML class namehighlight.It is consistent with the case of
hljs: true.And also, even if
hljs: false, the output ofhighlight()should be include a PRE element and a CODE element.It is consistent with the case of
hljs: true, too.