Add global line number option & hide line numbers by default for code blocks

[tlylt]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• Feature addition or enhancement
• Code maintenance
• Others, please explain:

Fixes #1671

Overview of changes:

• Make line numbers hidden by default
• Enable a global line number setting via codeLineNumbers in site.json
  • individual code blocks can still specify line-numbers or no-line-numbers for custom behavior
• Update test cases
• Update docs to mention the above adjustment
  • also removed unnecessary {.no-line-numbers}

Edit:
Based on the suggestions, code blocks that have {start-from=x} (x is the line number) will force line numbers to show even if the global setting is to hide it.

Anything you'd like to highlight / discuss:
After discussion, line numbers seem to take up too much space and add additional noise when code blocks display code outputs or other not-so-suitable situations. If enabled by default, this may be inconvenient for authors to hide line numbers one by one.
Thus,

• code line numbers are now hidden by default
• individual code blocks that do not have line-numbers or no-line-numbers class will inherit from the global setting (if not set, default is to hide all line numbers)
• if a line number class is set, the code block will behave accordingly

In terms of code, I processed this logic in core/src/html/NodeProcessor.js 's processNode, it should also be possible to process it in postProcessNode, I am not sure if there is any preference or requirement on this. At the moment the current way to append line-numbers/no-line-numbers seems functional.

Testing instructions:

• Functional test case testCodeBlock.md adjusted to test the behavior
• In user guide, the part about code block syntax is edited here, and
• the part about site.json is edited here

Proposed commit message: (wrap lines at 72 characters)
Adjust line numbers setting for code blocks

Line numbers are provided by default and there is no way
to override it globally.

Let's include a codeLineNumbers option in site.json to
enable a global configuration of line numbers. Line numbers
are also now hidden by default.

This is more convenient for authors as line numbers tend to
be hidden manually in practice. This is also consistent with other
markdown renderers.

---

Checklist: ☑️

• Updated the documentation for feature additions and enhancements
• Added tests for bug fixes or features
• Linked all related issues
• No unrelated changes

[ong6]

Perhaps you could add the "codeLineNumbers": true to the site.json so that people referencing that can easily see this feature?

Also I noticed that in the docs start-from=6 is not working?

Since its working on the main website I assume something you did might have caused start-from to stop being parsed correctly. For example, I think since the default is now to not show the line numbers, start-from also does not show them hence this issue. Perhaps adding a check for start from inside codeblockProcessor could fix this issue.

[tlylt]

Perhaps you could add the "codeLineNumbers": true to the site.json so that people referencing that can easily see this feature?

Added.

Also I noticed that in the docs start-from=6 is not working?

Since its working on the main website I assume something you did might have caused start-from to stop being parsed correctly. For example, I think since the default is now to not show the line numbers, start-from also does not show them hence this issue. Perhaps adding a check for start from inside codeblockProcessor could fix this issue.

Yup thanks for the thorough check, I indeed forgot about this case. At the moment I am adding .line-numbers to the affected code blocks (start-from and highlight-lines) in the user guide so that the line numbers do show up.

For start-from, it definitely has to go with line-numbers for it to make sense, but I am not sure whether I should explicitly allow it to work when the default is to not show line numbers. This is because the "right" way is to override line-number settings individually -> {.line-numbers start-from=6}. The alternative is to allow it to work without specifying .line-numbers when the default is to hide line numbers.

Similar logic for highlight-lines, except that it might still be ok without displaying line numbers. In the user guide, I added {.line-numbers} so that it's easier to see the effect of the highlighting, but it is totally fine for authors to want to hide the line numbers as it does not affect it's functionality.

Would like a second opinion on this as I am ok with both.

1. start-from will force code block to show line-numbers (and what about highlight-lines?)
2. start-from requires explicit .line-numbers when the global config is to hide line numbers by default.

[ong6]

Would like a second opinion on this as I am ok with both.

1. start-from will force code block to show line-numbers (and what about highlight-lines?)
2. start-from requires explicit .line-numbers when the global config is to hide line numbers by default.

I think that the first option is better for current users of markbind. This is my reasoning:

1. Existing users would have used start-from without the .line-number being set.
2. Ease of use, users won't need to remember to add a .line-number to every singe code block which they also want start-from.

Also I don't think this affects highlight-lines? It seems to be working fine even without the line numbers.

[jonahtanjz]

Nice work @tlylt 👍

Just a suggestion on the implementation.

What do you think of creating 2 separate css files for the different line number settings and import them respectively instead of processing each of the code block?

For example, if the style.codeLineNumbers is true in site.json,

• The default code block css rules will add line numbers.
• If there is .no-line-number class, then line numbers will not show.
  (The above is the same as how the css is implemeneted now)

If the style.codeLineNumbers is false or not specified in site.json,

• The default code block css rules will not add line numbers.
• If there is line-number class, then line numbers will be added.
  (The opposite of how it is implemented now)

These 2 css logics can be in 2 different css files (maybe show-default-line-number.css and no-default-line-number.css). Then in the Site/index.js, it will choose the respective css to import based on what is specified in the site.json (Can refer to the implementation of style.codeTheme here and here.

One reason for doing this is so that the implementation is simpler and faster. Not sure if this would create too many assets? Would like to hear your opinion on this.

Would like a second opinion on this as I am ok with both.

1. start-from will force code block to show line-numbers (and what about highlight-lines?)
2. start-from requires explicit .line-numbers when the global config is to hide line numbers by default.

I agree with @ong6 on this. Option 1 would be much better as it does not make sense to have start-from without line numbers. So having start-from would naturally imply that there should be line numbers. Also existing users will not need to do anything for their current start-from to work.

[tlylt]

What do you think of creating 2 separate css files for the different line number settings and import them respectively instead of processing each of the code block?

For example, if the style.codeLineNumbers is true in site.json,

• The default code block css rules will add line numbers.
• If there is .no-line-number class, then line numbers will not show.
  (The above is the same as how the css is implemeneted now)

If the style.codeLineNumbers is false or not specified in site.json,

• The default code block css rules will not add line numbers.
• If there is line-number class, then line numbers will be added.
  (The opposite of how it is implemented now)

These 2 css logics can be in 2 different css files (maybe show-default-line-number.css and no-default-line-number.css). Then in the Site/index.js, it will choose the respective css to import based on what is specified in the site.json (Can refer to the implementation of style.codeTheme here and here.

One reason for doing this is so that the implementation is simpler and faster. Not sure if this would create too many assets? Would like to hear your opinion on this.

Hmm, I didn't consider this approach initially as I was not aware of the code-theme implementation. I think implementation wise it does feel like having separate stylesheets might be faster as we don't have to check in the token processing step and add the style class(though this rough gauge of performance degradation is probably inaccurate and unwarranted, without the use of proper tools). I guess one minor con of doing it in separate stylesheets is that, unlike themes, code line numbers are probably only going to be two and hence may not enjoy the potential extensibility from the proposed approach.

By the way, a small question on the CSS files: I see that the code theme stylesheets you mentioned are minimized, by any chance do you know what's the tool used for that?

Would like a second opinion on this as I am ok with both.

1. start-from will force code block to show line-numbers (and what about highlight-lines?)
2. start-from requires explicit .line-numbers when the global config is to hide line numbers by default.

I agree with @ong6 on this. Option 1 would be much better as it does not make sense to have start-from without line numbers. So having start-from would naturally imply that there should be line numbers. Also existing users will not need to do anything for their current start-from to work.

Sure I will make the relevant changes for this part.

[jonahtanjz]

Hmm, I didn't consider this approach initially as I was not aware of the code-theme implementation. I think implementation wise it does feel like having separate stylesheets might be faster as we don't have to check in the token processing step and add the style class(though this rough gauge of performance degradation is probably inaccurate and unwarranted, without the use of proper tools). I guess one minor con of doing it in separate stylesheets is that, unlike themes, code line numbers are probably only going to be two and hence may not enjoy the potential extensibility from the proposed approach.

Alright in that case we can stick to the current implementation for now 👍

By the way, a small question on the CSS files: I see that the code theme stylesheets you mentioned are minimized, by any chance do you know what's the tool used for that?

I'm not sure which tool was used to minify those CSS files but I believe there are online tools available which you can upload/paste in the CSS code and it will generate a minimized version.

[ang-zeyu]

One reason for doing this is so that the implementation is simpler and faster. Not sure if this would create too many assets? Would like to hear your opinion on this.

I'm favouring @tlylt's existing implementation here as well.

Conditionally importing assets is quite a bit of work: https://github.com/MarkBind/markbind/blob/master/packages/core/src/Site/index.js#L1411
It also takes the css building out of our webpack bundling process which can be difficult down the line to debug / standardise.

We currently conditionally import different assets, but also only external ones (bootswatch / code dark themes).

One reason is that we'd have to include these (rather huge) assets into the webpack bundling process otherwise which increases build times & possibly tooling / configuration. (and we'd have a matrix of bundles then which isn't too build time friendly 🙁)

[tlylt]

Hi reviewers,
Thank you for the kind reviews and suggestions. I have adjusted the code and hopefully fixed all the pending issues accordingly.
The new changes made are:

• show line numbers if "start-from" is set
• use regex to match for 'line-numbers' or 'no-line-numbers'
• update test cases
• update CSS selector
• update code logic with regards to codeLineNumbers config
• minor update to the docs

[ang-zeyu]

last few nits, otherwise lgtm! 👍