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

feat(prism): add mark & firstLine option support #172

Merged
merged 4 commits into from Jan 13, 2020

Conversation

@SukkaW
Copy link
Member

SukkaW commented Jan 4, 2020

@SukkaW SukkaW requested review from seaoak and hexojs/core Jan 4, 2020
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 4, 2020

Coverage Status

Coverage increased (+0.03%) to 96.994% when pulling c356fed on SukkaW:prismhighlight-plugin into f6eaed9 on hexojs:master.

@seaoak

This comment has been minimized.

Copy link
Member

seaoak commented Jan 8, 2020

I have a question.

Currently, firstLine option will have effect only when lineNumbers=true.
This behavior describes clearly in README.

But I think some users might want to highlight lines with lineNumbers=false.

For example, with an options:

options = {
  isPreprocess: false,
  lineNumbers: false,
  firstLine: 101,
  mark: '103',
}

the user might intend to highlight the 3rd line.

It is achieved by adding the attribute data-line-offset="100" for pre element.
https://prismjs.com/plugins/line-highlight/

  if (!lineNumber && firstLine && !isPreprocess) preTagAttrArr.push(`data-line-offset="${firstLine-1}"`);

Is this feature useful or not ?

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Jan 8, 2020

@seaoak We should make sure that it will be consistent with highlight.js and won't break backtick_filter and code() helper.

@SukkaW SukkaW force-pushed the SukkaW:prismhighlight-plugin branch from ca97457 to b10357a Jan 8, 2020
@seaoak

This comment has been minimized.

Copy link
Member

seaoak commented Jan 9, 2020

@SukkaW I found that highlight() just behaves like above feature.

For example, with the options:

options = {
  gutter: false,
  firstLine: 101,
  mark: '103',
}

highlight() marks the 3rd line.
That is, firstLine option has effect even if gutter=false.

Certainly, in hexo:lib/plugins/filter/before_post_render/backtick_coode_block.js,
firstLine option is set only when gutter=true.
But, as a result, above feature has no effect for backtick_code_block filter (not break).

And also, in hexo:lib/plugins/tag/code.js,
firstLine option is independent of line_number option (= gutter option).
For example, a snippet below highlights the 3rd line.

{% codeblock line_number:false first_line:101 mark:103 %}

I think this feature will increase compatibility with highlight.js.
Could you consider this feature ?

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Jan 9, 2020

According to PrismJS documents:

In case you want the line numbering to be offset by a certain number (for example, you want the 1st line to be number 41 instead of 1, which is an offset of 40), you can additionally use the data-line-offset attribute.

@seaoak

So we should accept firstLine no matter lineNumbers enabled or not, and apply offset as long as firstLine is given?

@SukkaW SukkaW force-pushed the SukkaW:prismhighlight-plugin branch from 03c9008 to f8feb41 Jan 9, 2020
@seaoak

This comment has been minimized.

Copy link
Member

seaoak commented Jan 9, 2020

@SukkaW I agreed.

Could you add test case for generating the attribute data-line-offset ?

@SukkaW

This comment has been minimized.

Copy link
Member Author

SukkaW commented Jan 10, 2020

@seaoak Related test case added.

@seaoak
seaoak approved these changes Jan 13, 2020
@SukkaW SukkaW merged commit ddcdc31 into hexojs:master Jan 13, 2020
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.03%) to 96.994%
Details
@SukkaW SukkaW mentioned this pull request Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.