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

Make consistency in output of "highlight()" for "wrap: false" (fix #111) #112

Conversation

@seaoak
Copy link
Member

seaoak commented Oct 19, 2019

Fix #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 be include a PRE element and a CODE element.
It is consistent with the case of hljs: true, too.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 19, 2019

Coverage Status

Coverage increased (+0.02%) to 95.302% when pulling 62ab0f8 on seaoak:feature/make_consistency_in_output_when_wrap_false into 635161b on hexojs:master.

@seaoak

This comment has been minimized.

Copy link
Member Author

seaoak commented Oct 19, 2019

This PR will be used to introduce wrap option in highlight section of Hexo's _config.yml.

@seaoak seaoak requested a review from curbengh Oct 19, 2019
@@ -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

This comment has been minimized.

Copy link
@curbengh

curbengh Oct 19, 2019

Contributor

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 || '';.

This comment has been minimized.

Copy link
@seaoak

seaoak Oct 20, 2019

Author Member

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?

This comment has been minimized.

Copy link
@curbengh

curbengh Oct 22, 2019

Contributor

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.

This comment has been minimized.

Copy link
@seaoak

seaoak Oct 23, 2019

Author Member

@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 seaoak force-pushed the seaoak:feature/make_consistency_in_output_when_wrap_false branch from 73ce38f to a8de9f5 Oct 24, 2019
@seaoak

This comment has been minimized.

Copy link
Member Author

seaoak commented Oct 24, 2019

I updated the code of this PR (reflect @curbengh 's comments and add two test cases).
Could you review this again?

@seaoak seaoak requested a review from curbengh Oct 25, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Oct 27, 2019

Since wrap: false is not compatible with gutter: true, shall we give priority to "gutter" (i.e. enable "wrap" when "gutter" is enabled)? My preference is on "gutter".

Definitely we'll have to mention in the docs about the compatibility (after wrap option is introduced in hexo).

@seaoak

This comment has been minimized.

Copy link
Member Author

seaoak commented Oct 27, 2019

I agree with giving the priority (gutter option is superior to wrap option).

I will update this PR to arbitrate the conflict with them.

@seaoak seaoak force-pushed the seaoak:feature/make_consistency_in_output_when_wrap_false branch from a8de9f5 to 980adfd Oct 27, 2019
lib/highlight.js Show resolved Hide resolved
lib/highlight.js Show resolved Hide resolved
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 seaoak force-pushed the seaoak:feature/make_consistency_in_output_when_wrap_false branch from 980adfd to 62ab0f8 Oct 28, 2019
@seaoak

This comment has been minimized.

Copy link
Member Author

seaoak commented Oct 28, 2019

@curbengh I'm sorry it was an elementary mistake.
I updated this PR (adding a test case).

@curbengh curbengh requested a review from hexojs/core Oct 28, 2019
@curbengh curbengh merged commit 42b7393 into hexojs:master Nov 2, 2019
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.02%) to 95.302%
Details
@curbengh curbengh mentioned this pull request Nov 2, 2019
1 of 2 tasks complete
@curbengh curbengh mentioned this pull request Nov 19, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.