Add heading and line highlighting to code blocks #1034

nbriannl · 2020-02-09T04:27:10Z

~~Note: Will take out of WIP when #991 is approved~~

~~Note: This branch is a rebase over #991 in order to work on top of his changes. Will rebase over master once #991 is merged~~

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

• [X] Enhancement to an existing feature

Code headings fixes #646 #645
Partially fixes the requirements of line highlighting in #473

What is the rationale for this request?

For MarkBind to be a developer documentation tool, can be helpful to indicate the language of a code block in the block itself. To make the usage more general, any heading test can be optionally added to a code block, be it a file name & extension, the language used, or a generic heading.

Also, it would be good to be able to highlight specific lines of the code block, so to allow the author to emphasis certain lines.

What changes did you make? (Give an overview)

Because the following are markdown-it-attrs, you can use the original fenced code syntax if you do not want to use these feature.

We overwrite the markdown-it rule for fenced code blocks, to add a heading (which is a div) if a heading is specified as an attribute.

As for the highlight-lines attribute, firstly, the values are parsed into ranges. Secondly, we add upon the implementation of the map function in #991 that maps each lines into spans. What this addition is is that if the current line is within any of the ranges that is intended to be highlighted, it is then given the CSS class "highlighted". This also means that it is ok to give an invalid ranges, i.e. a line number that is out of bounds of the number of lines in the code, because the lines are checked against the ranges and not the other way around.

CSS changes were also made to enable styling and position of the heading div with respect to the div containing the code.

The headings are meant to be additional (and possibly unimportant) info. That's why they are on the right rather than left, away from the readers main path of reading.

Hence, the following design, here is how it looks like

Provide some example code that this change will affect:
Add the following syntax to any test site and serve

```xml
<foo>
  <bar type="name">goo</bar>
</foo>
```
```xml {heading="html"}
<foo>
  <bar type="name">goo</bar>
</foo>
```
```nginx {heading="Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat."}
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
}
```
```nginx {heading="This is like a box heading" highlight-lines="1,4-7,8,11-55"}
user  www www;
worker_processes  2;
pid /var/run/nginx.pid;
error_log  /var/log/nginx.error_log  debug | info | notice | warn | error | crit;

events {
    connections   2000;
    use kqueue | rtsig | epoll | /dev/poll | select | poll;
}

server { # simple reverse-proxy
    listen       80;
    server_name  domain2.com www.domain2.com;
    access_log   logs/domain2.access.log  main;
} 
# It is ok to specify a range greater than the lines of the code block
```

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

Please comment if the styling of the component can be improved.

Proposed commit message: (wrap lines at 72 characters)

Add heading and line highlighting to code blocks

alyip98 · 2020-02-09T14:03:23Z

I think it looks weird to have the heading box on the right, its usually on the left.

I personally prefer the look of the previous implementation more than the heading box

And we could add a bit of CSS to solve the previously mentioned issues

.code-block:hover > .code-block-lang {
    display: none;
}

damithc · 2020-02-09T14:09:18Z

I think it looks weird to have the heading box on the right, its usually on the left.

Heading is a misnomer; these are meant to be additional (and possibly unimportant) info. That's why they are on the right rather than left, away from the readers main path of reading.

And we could add a bit of CSS to solve the previously mentioned issues

What about when the page is converted to pdf?

damithc · 2020-02-09T14:12:16Z

In terms of saving vertical space, perhaps we can make the heading overlap with the box about half a line height? Not sure how it will look though. Can try it out.

nbriannl · 2020-02-09T14:12:57Z

Heading is a misnomer

Then, should it be given another name, like 'alt-text'?

damithc · 2020-02-09T14:16:08Z

Then, should it be given another name, like 'alt-text'?

I think we should. alt-text might not work though, as it already is used for a different purpose (in HTML).

alyip98 · 2020-02-09T14:18:19Z

What about when the page is converted to pdf?

We could use the .d-print-none class provided by Bootstrap to hide it from print.

damithc · 2020-02-09T14:21:29Z

We could use the .d-print-none class provided by Bootstrap to hide it from print.

The user may not want to hide it from the print.

nbriannl · 2020-02-10T04:09:26Z

In terms of saving vertical space, perhaps we can make the heading overlap with the box about half a line height? Not sure how it will look though. Can try it out.

This is how it looks like

nbriannl · 2020-02-10T04:47:56Z

As discussed, this is the final one.

marvinchin

Thanks for looking at this! ~~Perhaps you can consider changing the base branch to be #991 instead of master to make it easier to review subsequently.~~ I forgot that we work on different forks here 😅

marvinchin · 2020-02-11T08:20:08Z

asset/css/markbind.css

+    position: relative;
+}
+
+.code-block > .code-block-heading {


.code-block-heading sounds specific enough without having to further qualify it with .code-block >. Let's remove the unnecessary qualifiers.

marvinchin · 2020-02-11T08:22:59Z

docs/userGuide/syntax/code.mbdf

+</span>
+</include>
+
+##### Using them altogether


I feel that this section isn't very necessary, since combining selectors should be fairly self explanatory. I'd lean towards removing this section to avoid bloating the user guide.

Turns out we do need it 😅

marvinchin · 2020-02-11T08:26:04Z

src/lib/markbind/src/lib/markdown-it/index.js

+  let highlightLines = token.attrGet('highlight-lines')
+  let lineNumbers = []
+  if (highlightLines) {
+    lineNumbers = highlightLines.split(',').map(v => v.split('-').map(v => parseInt(v, 10))) 


Some comments here to describe what the input format should look like would be helpful! It would make maintaining this code easier in the future.

Splitting up this line into several steps with well-named intermediate variables could help as well.

marvinchin · 2020-02-11T08:29:09Z

src/lib/markbind/src/lib/markdown-it/index.js

+      return currentLineNumber === start
+    })
+    if (inRange) {
+      return `<span class="highlighted">${line || '&#x200B;'}</span>`;


I think we can abstract the section ${line || ''} into a separate variable, to make it clearer that this is identical across both cases.

marvinchin · 2020-02-11T08:29:33Z

src/lib/markbind/src/lib/markdown-it/index.js

@@ -45,17 +45,44 @@ markdownIt.renderer.rules.fence = (tokens, idx, options, env, slf) => {
  if (!highlighted) {
    str = markdownIt.utils.escapeHtml(str);
  }
+  let highlightLines = token.attrGet('highlight-lines')


You have missing ;s in this file.

Done. Turns out eslint ignores src/lib/markbind/src/lib/markdown-it/* 😱

marvinchin · 2020-02-11T08:32:02Z

src/lib/markbind/src/lib/markdown-it/index.js

+  if (heading) {
+    return `<div class='code-block'>`
+      + `<div class='code-block-heading'><span>` + heading + `<span></div>` 
+      + `<div class='code-block-content'><pre><code ${slf.renderAttrs(token)}>${str}</code></pre></div>`


Same as above, lets abstract the common part into it's own variable.

marvinchin

I was playing around with the feature and found an issue when the heading is too long:

Perhaps we could either split it across several lines, or truncate it after a certain length?

marvinchin · 2020-02-13T14:54:21Z

src/lib/markbind/src/lib/markdown-it/index.js

@@ -45,17 +45,55 @@ markdownIt.renderer.rules.fence = (tokens, idx, options, env, slf) => {
  if (!highlighted) {
    str = markdownIt.utils.escapeHtml(str);
  }
+
+  const linesToHighlight = token.attrGet('highlight-lines');


What do you think of this?
linesToHighlight -> highlightLinesInput
arrLinesToHighlight -> highlightLines

This is to make it clearer which is the raw input that needs to be processed, and which are the actual lines to be highlighted.

I generally also don't like using arr to denote arrays as it leads to unnecessary verbosity IMO. A well named plural variable should be sufficient to convey that.

Looks good to me.

src/lib/markbind/src/lib/markdown-it/index.js

nbriannl · 2020-02-13T16:00:46Z

I was playing around with the feature and found an issue when the heading is too long:

Perhaps we could either split it across several lines, or truncate it after a certain length?

overflow-wrap: break-word; does the trick.

nbriannl · 2020-02-26T03:24:16Z

#991 has been merged. Ready for review.

marvinchin · 2020-02-26T17:04:02Z

src/lib/markbind/src/lib/markdown-it/index.js

+  const heading = token.attrGet('heading');
+  const codeBlockContent = `<pre><code ${slf.renderAttrs(token)}>${str}</code></pre>`;
+  if (heading) {
+    return `${'<div class=\'code-block\'>'


The nested string interpolation feels a little strange to me. I normally do prefer string interpolation to concatenation, but for multi-line strings it gets difficult to read. What do you think of this?

return '<div class="code-block">' + `<div class="code-block-heading"><span>${heading}<span></div>` + `<div class="code-block-content">${codeBlockContent}</div>` + '</div>';

Sorry if this diff was already included when I last reviewed it and I didn't catch it then 🙏

marvinchin

LGTM 🚀

yamgent · 2020-03-03T13:48:58Z

@nbriannl will need you to update the css files, thanks.

Added heading to code blocks Add line highlighting to code blocks Edit css remove unit for length 0 css (cherry picked from commit da7e23a) Update User Guide (cherry picked from commit aceb551) Remove console log (cherry picked from commit cd8cc26) Update User Guide Update User Guide Update tests Edit CSS Edit CSS Remove User Guide Section Remove uneeded specifity Rewrite code for clarity Edit User Guide Handle wrap for very long single word Improve variable naming Update test sites Add note for optimization Settle rebase Reduce complexity of code sample in User Guide rewrite return string

nbriannl · 2020-03-03T14:31:47Z

CSS files updated. @yamgent

damithc · 2020-03-07T08:46:50Z

@nbriannl the netlify preview of this PR has some hover over glitches. Are they being addressed elsewhere?

nbriannl · 2020-03-07T08:52:54Z

@damithc what is the hover over glitch specifically?

damithc · 2020-03-07T11:06:45Z

The page width seem to change as I scroll the page. In the top half of the page, the middle column seem to be wider than intended, which pushes the pageNav gets out of view (as seen from the screenshot) but things go back to normal in the 2nd half of the page. Scroll that specific page in the user guide from top to bottom while watching the width of the middle column. Let me know if you can't reproduce it.

nbriannl · 2020-03-07T11:12:48Z

I can reproduce it in this PR. #1077 has been raised regarding this.

damithc · 2020-03-07T11:15:28Z

I can reproduce it in this PR. #1077 has been raised regarding this.

Got it. Seems severe enough problem that needs to be fixed before the next release though. Are you working on it?

nbriannl · 2020-03-07T11:17:47Z

I was not working on it and this is the first time I'm aware regarding this. I'm investigating it now.

…nvert-to-code-block * 'master' of https://github.com/MarkBind/markbind: Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062) # Conflicts: # docs/userGuide/syntax/siteNavigationMenus.mbdf

* 'master' of https://github.com/MarkBind/markbind: 2.12.0 Update outdated test files Update vue-strap version to v2.0.1-markbind.37 Fix refactor to processDynamicResources (MarkBind#1092) Implement lazy page building for markbind serve (MarkBind#1038) Add warnings for conflicting/deprecated component attribs (MarkBind#1057) Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062)

nbriannl changed the title ~~[WIP] Add heading to code blocks and highlights~~ [WIP] Add heading to code blocks and line highlights Feb 9, 2020

nbriannl changed the title ~~[WIP] Add heading to code blocks and line highlights~~ [WIP] Add heading to code blocks and line highlighting Feb 9, 2020

nbriannl changed the title ~~[WIP] Add heading to code blocks and line highlighting~~ [WIP] Add heading and line highlighting to code blocks Feb 9, 2020

nbriannl force-pushed the code-heading-highlights branch 2 times, most recently from 8e578eb to 90df946 Compare February 9, 2020 09:23

nbriannl requested a review from openorclose February 9, 2020 09:40

nbriannl changed the title ~~[WIP] Add heading and line highlighting to code blocks~~ Add heading and line highlighting to code blocks Feb 9, 2020

nbriannl requested review from marvinchin and yamgent February 9, 2020 12:20

nbriannl force-pushed the code-heading-highlights branch from aa87b47 to 0c8df0a Compare February 11, 2020 03:19

marvinchin requested changes Feb 11, 2020

View reviewed changes

nbriannl requested a review from marvinchin February 11, 2020 10:37

marvinchin requested changes Feb 13, 2020

View reviewed changes

openorclose reviewed Feb 13, 2020

View reviewed changes

src/lib/markbind/src/lib/markdown-it/index.js Show resolved Hide resolved

nbriannl requested a review from marvinchin February 13, 2020 16:04

nbriannl changed the title ~~Add heading and line highlighting to code blocks~~ [WIP] Add heading and line highlighting to code blocks Feb 15, 2020

nbriannl force-pushed the code-heading-highlights branch from ae4b108 to 2b9d5c4 Compare February 26, 2020 03:09

nbriannl requested a review from openorclose February 26, 2020 03:29

nbriannl changed the title ~~[WIP] Add heading and line highlighting to code blocks~~ Add heading and line highlighting to code blocks Feb 26, 2020

nbriannl force-pushed the code-heading-highlights branch from 3c03216 to 066ddcf Compare February 26, 2020 09:34

marvinchin reviewed Feb 26, 2020

View reviewed changes

nbriannl requested a review from marvinchin February 27, 2020 03:45

marvinchin approved these changes Feb 27, 2020

View reviewed changes

marvinchin added this to the v2.11.1 milestone Feb 27, 2020

nbriannl mentioned this pull request Feb 28, 2020

Add dividers and fix bug in siteNav #1063

Merged

nbriannl force-pushed the code-heading-highlights branch from 246ee90 to 4feb2e9 Compare March 3, 2020 14:04

nbriannl force-pushed the code-heading-highlights branch from 4feb2e9 to abdff1e Compare March 3, 2020 14:13

nbriannl added 3 commits March 3, 2020 22:17

update tests

f9f754d

add blank line at eol

17023a2

update css order and test sites

1198805

yamgent approved these changes Mar 7, 2020

View reviewed changes

yamgent added the pr.Enhancement 📈 Enhancement to an existing feature label Mar 7, 2020

yamgent merged commit 3a41773 into MarkBind:master Mar 7, 2020

damithc mentioned this pull request Mar 7, 2020

Difference in the way shown to the user #1091

Closed

nbriannl mentioned this pull request Mar 7, 2020

[For Testing] Add pageNav to Using Plugins Page (#1062) #1094

Closed

marvinchin pushed a commit that referenced this pull request Apr 10, 2020

Add heading and line highlighting to code blocks (#1034)

65809a9

nbriannl deleted the code-heading-highlights branch April 16, 2020 09:55

nbriannl mentioned this pull request Apr 29, 2020

Add and organise tests for code blocks #1143

Merged

Add heading and line highlighting to code blocks #1034

Add heading and line highlighting to code blocks #1034

Conversation

nbriannl commented Feb 9, 2020 • edited Loading

alyip98 commented Feb 9, 2020

damithc commented Feb 9, 2020

damithc commented Feb 9, 2020

nbriannl commented Feb 9, 2020

damithc commented Feb 9, 2020

alyip98 commented Feb 9, 2020

damithc commented Feb 9, 2020

nbriannl commented Feb 10, 2020

nbriannl commented Feb 10, 2020

marvinchin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbriannl commented Feb 13, 2020

nbriannl commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinchin left a comment

Choose a reason for hiding this comment

yamgent commented Mar 3, 2020

nbriannl commented Mar 3, 2020

damithc commented Mar 7, 2020

nbriannl commented Mar 7, 2020

damithc commented Mar 7, 2020

nbriannl commented Mar 7, 2020

damithc commented Mar 7, 2020

nbriannl commented Mar 7, 2020

nbriannl commented Feb 9, 2020 •

edited

Loading

marvinchin left a comment •

edited

Loading