Parity with SyntaxHighlight extension #7
Conversation
|
@Nicolas01 Have you had a chance to take a look at this? Having support for all the same features as the SyntaxHighlight extension would allow more wikis to migrate to this extension instead. |
|
@kuenzign Thanks for the good work. |
| } | ||
|
|
||
| $code = htmlspecialchars(trim($in)); | ||
| $out = htmlspecialchars($out); |
There was a problem hiding this comment.
I would move the strip markers replacement and the rtrim here:
| $out = htmlspecialchars($out); | |
| // Replace strip markers (For e.g. {{#tag:syntaxhighlight|<nowiki>...}}) | |
| $out = $parser->mStripState->unstripNoWiki( $in ); | |
| $out = htmlspecialchars( rtrim( $out ) ); |
|
|
||
| // Convert deprecated attributes | ||
| if (isset($param['enclose'])) { | ||
| if ($param['enclose'] === 'none') { | ||
| $param['inline'] = true; | ||
| } | ||
| unset($param['enclose']); | ||
| } |
There was a problem hiding this comment.
As you mentioned, enclose is a deprecated attribute.
So it may not be a good idea to handle it anymore.
| // Convert deprecated attributes | |
| if (isset($param['enclose'])) { | |
| if ($param['enclose'] === 'none') { | |
| $param['inline'] = true; | |
| } | |
| unset($param['enclose']); | |
| } |
|
|
||
| // Don't trim leading spaces away, just the linefeeds | ||
| $out = preg_replace('/^\n+/', '', rtrim($out)); |
There was a problem hiding this comment.
Good point. I would only do a rtrim and leave the first empty lines.
I would also move the rtrim to the line 67.
| // Don't trim leading spaces away, just the linefeeds | |
| $out = preg_replace('/^\n+/', '', rtrim($out)); |
| // Replace strip markers (For e.g. {{#tag:syntaxhighlight|<nowiki>...}}) | ||
| $out = $parser->mStripState->unstripNoWiki($in); |
There was a problem hiding this comment.
I would move the strip markers replacement to the line 67.
| // Replace strip markers (For e.g. {{#tag:syntaxhighlight|<nowiki>...}}) | |
| $out = $parser->mStripState->unstripNoWiki($in); |
| // Allow certain HTML attributes | ||
| $htmlAttribs = Sanitizer::validateAttributes( | ||
| $param, | ||
| array_flip(['line', 'start', 'highlight', 'style', 'class', 'id', 'dir']) | ||
| ); | ||
|
|
| // id | ||
| if (isset($param['id'])) { | ||
| $htmlAttribs['id'] = $param['id']; | ||
| if (!(isset($htmlAttribs['dir']) && $htmlAttribs['dir'] === 'rtl')) { |
There was a problem hiding this comment.
Is there a need to force the dir attribute to ltr?
| @@ -15,12 +15,86 @@ $(document).ready(function () { | |||
| // load the highlight.min.js script this way to avoid Uncaught SyntaxError: unterminated regular expression literal | |||
| $.cachedScript('https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.5.1/highlight.min.js') | |||
| .done(function (script, textStatus) { | |||
| $.cachedScript('https://cdnjs.cloudflare.com/ajax/libs/highlightjs-line-numbers.js/2.8.0/highlightjs-line-numbers.min.js') | |||
There was a problem hiding this comment.
This is my main concern about your PR:
Highlight.js doesn't manage line numbers to keep simple, and I have the same wish for this extension.
But as your already made the work, I would suggest you to create 2 separate PRs for this concern:
- one for the line numbers
- another one for the highlight of specific lines
There was a problem hiding this comment.
On second thought, I think you should create a separate extension for this.
highlightjs-line-numbers is a different projet than highlightjs so it makes sense for me to have separate extensions.
Nicolas01
force-pushed
the
master
branch
2 times, most recently
from
bbb82b1 to
2519aec
Compare
This enables support for all the same features as the SyntaxHighlight extension. This fixes #3, fixes #4, fixes #5, fixes #6, and fixes #8.