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

Support line numbers for code blocks #991

Merged
merged 19 commits into from
Feb 23, 2020
Merged

Conversation

openorclose
Copy link
Contributor

@openorclose openorclose commented Jan 20, 2020

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [x] Enhancement to an existing feature
• [ ] Other, please explain:

Partially fixes #473 (only adding support for line nos),

Fixes #1023

What is the rationale for this request?

What changes did you make? (Give an overview)

Small bugfix:

Our code fences (```) do not support markdown-it-attrs even though one of the examples there shows it should.

Turns out this is due to our overriding of the syntax highlighting.

image

We copied the second example from markdown-it, so that we could add our own hljs class to the <pre> tag, but this breaks markdown-it-attrs since the attrs there do not get added to our hardcoded pre tag.

So instead we provide our own renderer to render the attributes parsed by markdown-it-attrs.

Actual implementation

There is no way to directly use CSS and determine when line breaks happen, so we change highlightLines in markdown-it to split every \n and wrap a span around them.

Then, we use some css to insert the line numbers to it.

Line numbers will be shown by default, and authors can disable it by adding the class no-line-numbers.

Provide some example code that this change will affect:

```js
function f(n) {
  if (n < 1) {
    return 1;
  } else {
    return f(n - 1) + f(n - 2);
  }
}
f(10); // returns 12345;
```

Before and After:

image

If line numbers are not wanted:

```js {.no-line-numbers}
function f(n) {
  if (n < 1) {
    return 1;
  } else {
    return f(n - 1) + f(n - 2);
  }
}
f(10); // returns 12345;
```

Is there anything you'd like reviewers to focus on?

Check out the 2103 textbook here, where all code snippets have line numbers:
(the search on top doesn't work, it will bring you to the actual 2103 book site)
https://openorclose.github.io/se-book/

These pages have many code snippets:

Testing instructions:
Verify that <span>s get added to html of code fences, and manually check generated code fences for line numbers.

Proposed commit message: (wrap lines at 72 characters)

Add line numbers for code blocks

Let's wrap each line of a code block in a `<span>`,
and then add relevant css styles to display line numbers beside.

Line numbers will be displayed by default.

To turn off line numbers, add the class `no-line-numbers`
to a code block via markdown-it-attrs:

```js {.no-line-numbers}
const js = 1;
```

// TODO when design finalised

@damithc
Copy link
Contributor

damithc commented Jan 21, 2020

Nice. Can we make it the default? i.e., all fenced code blocks will have line numbers by default.

@openorclose
Copy link
Contributor Author

Nice. Can we make it the default? i.e., all fenced code blocks will have line numbers by default.

Sure, then users can disable it similarly (or add any other classes they want) by doing {.no-line-numbers}?

@yamgent
Copy link
Member

yamgent commented Jan 21, 2020

Sure, then users can disable it similarly (or add any other classes they want) by doing {.no-line-numbers}?

Yes please do provide the option to disable it.

@openorclose
Copy link
Contributor Author

Ok changes made,

Now all code fences that specify a language will have numbered lines.

Add the class no-line-numbers to disable.

@@ -297,3 +297,28 @@ panel[minimized]:not([expanded])::before {
li.footnote-item:target {
background-color: #eee;
}

pre {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this rule be changed to apply on .hljs instead? I doubt it will have an observable effect but it would be better practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean pre.hljs instead?

@alyip98
Copy link
Contributor

alyip98 commented Jan 22, 2020

What do you think of supporting arbitrary starting line numbers? I could see some use cases where the author wants to show a short code snippet without showing the whole file.

@yamgent
Copy link
Member

yamgent commented Jan 23, 2020

This PR is breaking the rendering of some code snippets, and also some scrollbars just appearing randomly:

image

@openorclose
Copy link
Contributor Author

This PR is breaking the rendering of some code snippets, and also some scrollbars just appearing randomly:

image

When a language is not specified, the .hljs class is not applied by markdown it, which is probably why the old solution was used in the first place to add the .hljs class to code fences with no language. Bc

But reverting the change would break markdown-it-attrs for code fences.

@yamgent
Copy link
Member

yamgent commented Jan 24, 2020

But reverting the change would break markdown-it-attrs for code fences.

Is it possible for MarkBind to add the class back in after all markdown syntax has been rendered (i.e. some post-rendering processing)?

@openorclose
Copy link
Contributor Author

Is it possible for MarkBind to add the class back in after all markdown syntax has been rendered (i.e. some post-rendering processing)?

I made code highlighting a new renderer rule instead of using the highlight property from markdown-it, so it works fine now.

image.

This allows us to manually render the attributes parsed from markdown-it-attrs.

What do you think of supporting arbitrary starting line numbers? I could see some use cases where the author wants to show a short code snippet without showing the whole file.

This would require a separate markdown plugin, as it is not possible (i.e. not nice) with pure css.
I would do this along with the rest of #473 by introducing some new syntax like markdown-it-attrs in a separate PR.

On a separate note, why do inline styles not work with markdown it attrs?

```js {data=123 style="counter-set:90;"}
function f(n) {
  if (n < 1) {
    return 1;
  } else {
    return f(n - 1) + f(n - 2);
  }
}
f(10); // returns 12345;
```

I tried doing this,

image

The inline style does get parsed and rendered in renderAttrs, but gets stripped out in the final html:

image

I tried it with **hi** {style = "color:red"} too and the the style doesn't work there too.

@damithc
Copy link
Contributor

damithc commented Jan 29, 2020

@openorclose
Copy link
Contributor Author

This page seems to be broken https://deploy-preview-991--markbind-master.netlify.com/userguide/reusingcontents

Forgot to manually escape code blocks without language, Thanks!

@openorclose openorclose changed the title [RFC] Support line numbers for code blocks Support line numbers for code blocks Jan 29, 2020
@openorclose
Copy link
Contributor Author

Ready for review!

@damithc
Copy link
Contributor

damithc commented Feb 2, 2020

Good work so far; looking forward to using this feature.
In some cases, there's a lot of space between the numbers and the code?
image

@openorclose
Copy link
Contributor Author

openorclose commented Feb 2, 2020

Good work so far; looking forward to using this feature.
In some cases, there's a lot of space between the numbers and the code?
image

I set it to to the size of 3 digits, since that's probably enough for most purposes:

This is what it looks like if I set the width to 2 digits:
image

What it looks like now:
image

To solve the space issue when there is only one digit, I changed the text to align right instead:
image

@@ -298,7 +298,7 @@ li.footnote-item:target {
background-color: #eee;
}

pre {
pre.hljs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class .hljs is applied to the <code> inside of <pre>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! thanks

@openorclose
Copy link
Contributor Author

Some parts of the documentation have line numbers, but we don't really want that because they are not code (e.g. https://deploy-preview-991--markbind-master.netlify.com/userguide/). Let's disable them.

Thanks! I've edited them, looked for instances of ``` and couldn't find other similar pages.

@openorclose
Copy link
Contributor Author

openorclose commented Feb 9, 2020

Fixes #1017, too with the added

pre {
    white-space: pre-wrap;
}

suggested in that issue

Before hovering:

image

After hovering:
image

@yamgent
Copy link
Member

yamgent commented Feb 15, 2020

There are some additional rendering problems in the documentation, can you please take a look at these:

(Both of these pictures are in userGuide/formattingContents.html, left side is this PR, right side is the correct version)

blockquotes

rules

@openorclose
Copy link
Contributor Author

openorclose commented Feb 15, 2020

There are some additional rendering problems in the documentation, can you please take a look at these:

(Both of these pictures are in userGuide/formattingContents.html, left side is this PR, right side is the correct version)

blockquotes

rules

Thanks!

This is due to hljs's rendering into <span>s sometimes containing line breaks:
image

*****
-----

gets rendered as one single span, so we can't just split the lines after highlighting.

So I decided to split the lines, then perform highlighting instead.

This causes a minor discrepancy with blockquotes (the single > is not highlighted, but then the following >> line is not highlighted as well
image

Horizontal and vertical spaces are fixed
image

@yamgent yamgent modified the milestones: v2.10.1, v2.10.2 Feb 23, 2020
@yamgent yamgent added breakingChange 💥 Feature will behave significantly different, or is made obsolete pr.Enhancement 📈 Enhancement to an existing feature labels Feb 23, 2020
@yamgent yamgent merged commit 5afcaa9 into MarkBind:master Feb 23, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Feb 27, 2020
* 'master' of https://github.com/MarkBind/markbind:
  Update tests
  Allow using 'none' footer attribute in frontmatter (MarkBind#1002)
  Support line numbers for code blocks (MarkBind#991)
  2.11.0
  Update test files due to changes in PR MarkBind#982
  Update vue-strap version to v2.0.1-markbind.36
  Make highlighting bold (MarkBind#1045)
  Support markdown for header attr in dropdown (MarkBind#1029)
  Add '_site' to the ignored folders in site.json (MarkBind#1046)
  Use path.join instead of string interpolation (MarkBind#1052)
  Implement box markdown header attributes parsing (MarkBind#1025)
  Make the position of top navbar fixed (MarkBind#982)
  Exclude *.md files from being copied over on build (MarkBind#1010)

# Conflicts:
#	docs/css/main.css
@damithc
Copy link
Contributor

damithc commented Mar 9, 2020

Works in production. Nice! 👍 Thanks @openorclose and reviewers. Let's push ahead and make our code blocks the best of class!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakingChange 💥 Feature will behave significantly different, or is made obsolete pr.Enhancement 📈 Enhancement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

markdown-it-attrs not working for code fences Support annotations in code examples
5 participants