Add line numbers library #46
Conversation
- Downloaded code-highlight-linenums dependency to count lines on vendor directory. - Added code highlight call and also loading of library - CodeHighlightLinenums is an optional library, so it can be avoided loading - Added css for line numbering. Added shim for code-highlight-lineums too. Indented better the coding with line numbers - Scoped new line-numbers CSS code - Added documentation on README.md and CHANGELOG.md
There was a problem hiding this comment.
Thanks, this is cool.
As a positive side-effect, it actually improves our fastboot compatibility too -- the prior code path doesn't run the source highlighter in fastboot (since didInsertElement does not run there, by design). If we go with this technique, I think we should just make it work that way even when lineNumbers==false, for consistency.
The use of triple-curlies made me question the security of this change -- it seems like it would possibly open us up to XSS vulnerabilities inside highlight.js or code-highlight-linenums.js. But then I went and read the source of highlight.js and realized that if it has vulnerabilities, we were already going to be pwned by them, because it's effectively doing the same thing as your code anyway: munging strings and then using innerHTML.
So this change doesn't negatively impact the security of ember-code-snippet users. We all just need to trust the escaping in highlight.js.
| Highlight.highlightBlock(this.get('element')); | ||
| if(!this.get('lineNumbers')) { | ||
| Highlight.highlightBlock(this.get('element')); | ||
| } |
There was a problem hiding this comment.
For consistency, let's eliminate didInsertElement and always render {{{source}}}, regardless of whether lineNumbers is true or false.
Otherwise the fastboot behavior is different based on whether you set lineNumbers, which would be surprising.
| <code class="hljs"> | ||
| {{#if lineNumbers}} | ||
| {{{source}}} | ||
| {{else}}{{source}}{{/if}} |
There was a problem hiding this comment.
To go with the previous comment about didInsertElement: let's remove this conditional and just always do the things that it does when lineNumbers=true.
| @@ -0,0 +1,96 @@ | |||
| (function(root) { | |||
There was a problem hiding this comment.
Please add a comment like
Copied from https://github.com/OverZealous/code-highlight-linenums v0.2.1
MIT Licensed
ensuring of course that that's really the right URL and version number.
| width: 6ch; | ||
| margin-left: calc(-6ch - 27px); | ||
| } |
There was a problem hiding this comment.
This whole file won't be included if the user chooses to install their own theme. Maybe that's OK? But if they install stock highlight.js themes they will lack styling for the line numbers.
An alternative would be to make this a separate CSS file that gets included whenever the code numbers JS file also gets included. If this CSS works well with many different highlight.js themes, that would probably be best.
|
Given the (substantial) API changes in #62, especially the fact that code highlighting responsibility has been moved out of this addon, I think this will need to be closed! BTW, see ember-bootstrap/ember-bootstrap#819 for an example implementation based on |
|
Thanks @simonihmig ! And sorry for not looking more closely my PR @ef4 |
directory.
loading
Example screenshot with lineNumbers and without: