Onebox: Github blob - Line Number


(lid) #1

I started off fixing the wrong lines bug and I got carried a way and introduced an option to also show line numbers.

Thinking I can just add line numbers in the beginning of each line and off I go, everybody happy :blush:
Highlight.js is the library used by Discourse to highlight code block on the fly. It is a pretty nice and powerful library but it does not support any sort of line numbering functionality. Adding the line number as I learnt breaks the lingual detection and syntax highlighting.

###First Try
My first approach was to try to trick highlight.js to treat my line numbers as line comments.
so I created a table, placed the line number on the 2nd column. and using css float the line number back to the left.

td.code {
float:right;
}
td.line_number{
float:left;
}

This solution sort of worked, but it is not the ideal solution. things will go ugly when the window width scale down.
And highlight.js will count the comment when it decide which language it is highlighting.

###Second Try / Approach
In the onebox template, instead of adding the line number as a content, I added it as an attribute to the column.

Using a delayed jQuery procedure with the hope that it will be enough for highlight.js finished processing my onebox. I Then change the content of the TD to its “line_number” attribute value.

This way we don’t have to use CSS trick and dangerous floats and word wrapping. We can use a proper stable markup. and highlight will have the proper content to work with. The only challenge here is to make sure the delay is sufficient.

I liked this overall approach, but we do have to inject JS from onebox.
with delay and such.

<table>
<tr>
<td line_number="5"></td>
<td>Some Code Line</td>
</tr>
</table>

##Final approach
I asked myself is there a better way? can we avoid using JavaScript? at least fundamentally?
You didn’t read this far just to hear “no” did you ?

So yes, we can have a css a pure css solution that will give us the following and will allow highlight.js
to properly highlight the syntax.

I did want to give Onebox a more robust way to be embedded without the complication of custom or external css dependencies.

$.onebox client Helper

So I wrote an helper for onebox to load embedded styling.
Now the problem with onebox is that we can have more then one box on the same page.
that mean that the code below will run for each box. but the helper is smart enoghe to only add a style once.

We can also add the $.onebox definition in the onebox template. and it will only be defined if not defined previously.

//define $.onebox here or externally
cssRule =  "b { color:red} ";
$.Onebox.loadStyle("make_bold_red",cssRule);

The full source of the $.onebox helper can be seen here.

###Onebox Githob blob using $.onbox helper

The CSS rule is a little bit lengthier, and this is how it will look.
So we are using js to load styling, but the number are actually generated by CSS
and an Ordered List.

if you are curious you can have a look here how it works. this is basically the code that will be used by the onebox.
http://jsfiddle.net/lid0/qa6dvpzo/

An Interesting fetug is that empty lines does not show, this is why for example we jump from line 228 to line 231 .
it makes code more compact :smirk:

Now all that is left is to actually push this PR and hope that it doesn’t fail :facepunch:

I personally think that syntax highlight could have moved to the ruby side, but it will add more dependency to Discourse / Onebox. The advantages is that the client side will have much less efforts rendering pages with code. specially for low end devices.

rouge A pure-ruby code highlighter that is compatible with pygments

Continuing the discussion from Github onebox shows wrong lines:


#2

Very nice. Keep coming with relevant/specific onebox stuff. Its interesting to read.


(Sam Saffron) #3

I appreciate all the work here but this is feeling a bit to fancy to me. I really don’t want us to support returning JS helpers with oneboxes it feels like it is not really its responsibility, it should be something you opt-in for.

Instead why not just simply add a data attribute and the consumers can decide what to do with it. So, for example:

<pre><code data-highlight-row="201" data-start-row="200">line1
line2
line3</code></pre>

This is the simplest solution, does not clash with anything and we can easily clean up the handling on our side without mucking with onebox gem.

I am concerned this PR is getting a bit too big now and its going to be hard to maintain this code.

Can you amend to use the above sample, then we can look at extending our highlight helper to support this data attribute. Further more we can then whitelist this attribute and others can use it to highlight anything, which is awesome.

Perhaps eventually adding something like

```javascript(hl=3,row_numbers)
function() {
    // this snippet will show row numbers
    thisIsHighlighted();
} 
```

(lid) #4

As to your recommendation I have simplified the solution.

With additional improvement: an indentation compactor.
https://github.com/discourse/onebox/pull/239

currently the “consumer” can extract line number from the ol start attribute element

<pre class="onebox">
<code class="">
<ol class="start lines" start="16" style="counter-reset: li-counter 15 ;">
...
</ol>
</code></pre>

And it can use some css love out of the box


(Sam Saffron) #5

Looks good, but we really need to fix styling here @Lid can you push through a PR to get line heights and padding and highlighting good.


(lid) #6

I have actually included a sample styling in the template.
which I now see missing a rule

pre.onebox code {
 white-space: normal;           
}

Example of a styled box


###Proposed Styling

This version is slightly different then the sample in the template, and it support an optional rule to render with the "N: " format.

@sam

  1. How would you like to go about adding the rules?
/** optional Better counter format **/
pre.onebox code ol.lines li:before {
list-style-type: none;
position: absolute;
left: -45px;
content: counter(li-counter) ':';
counter-increment: li-counter;
background: #EFEFEF;
color: #AFAFAF;
width: 45px;
display: inline-block;
text-align: right;
}

/** Technically can be implemented as an inline styling at template, but will produce a bigger footprint for the output  and will complicate the template **/ 
pre.onebox code ol{
	margin-left:0px;
}
pre.onebox code ol.lines{
	position:relative;
	margin-left: 45px;
}
pre.onebox code ol.lines li {
	list-style-type: decimal;
	padding-left: 6px;
	margin-left: 0px;

}
pre.onebox code li{
	background-color:#fff;
	border-bottom:1px solid #F0F0F0;
	padding-left:5px;

}
pre.onebox code li.selected{
	background-color:#cfc
}
pre.onebox code {
 white-space: normal;           
}


(Sam Saffron) #7

As a general rule, onebox should include no styling, it’s inflexible and not even valid html.

Just add the expected rules in a PR to discourse and document the sample styles in the onebox source tree


(lid) #8
  1. which file you want to add the rules to?

  2. I have left basic styling sample as template comment that will not render out.
    Is there a better place to document template related aspects?

What do you mean document it in the source tree?


(Sam Saffron) #9

In discourse place it here: discourse/app/assets/stylesheets/common/base at master · discourse/discourse · GitHub in a file called onebox.scss (I think most the onebox rules are on topic post, but this is a better spot) you will need to ensure its included as well

In onebox place all examples / doco in README.md


(lid) #10

pushed styling to onebox.scss, I think it is being included

https://github.com/discourse/discourse/pull/2823


(Sam Saffron) #11

I am liking the new styling thanks heaps.


(Sam Saffron) #12