Fixes to compatibility of line number and line higlight plugins #1194
Conversation
| */ | ||
| var _resizeElement = function (element) { | ||
| var codeStyles = getStyles(element); | ||
| var code = element.querySelector('code'); |
There was a problem hiding this comment.
This is redundant with codeElement declared a few lines below.
| @@ -102,13 +109,37 @@ | |||
| lineNumbersWrapper.className = 'line-numbers-rows'; | |||
| lineNumbersWrapper.innerHTML = lines; | |||
|
|
|||
| if (pre.hasAttribute('data-start')) { | |||
| pre.style.counterReset = 'linenumber ' + (parseInt(pre.getAttribute('data-start'), 10) - 1); | |||
| if (pre.hasAttribute('data-line-numbers-start')) { | |||
There was a problem hiding this comment.
I understand why you want to rename this data attribute... but it looks quite BC-breaking to me. We would need to update the docs, etc.
I would suggest you do the exact opposite: rename the data attribute used in line-highlight. AFAIK it's only used for the CSS ;)
|
No need to be sorry at all. And a huge thanks for diving into this! |
|
@Golmote Thanks! PR fixed. I've revert rename and, also, found that we can even don't rename it at all - anyway if these plugins used together, line highlight won't need And thanks for showing me that silly mistake, removed it. |
|
So, guys, what do you say? Should we wait two years for this one? :D |
|
|
||
| number = Math.abs(lineNumberStart) + number; | ||
|
|
||
| return lineNumberRows.children[number]; |
There was a problem hiding this comment.
@VitaliyR I think there is something wrong here. children is zero-indexed, while number (a value from data-line) is one-indexed. I tested in local and the highlighted line is indeed shifted down by one when line-numbers is used.
|
@Golmote thanks for finding! Indeed, I've encountered a bug. So, I've pushed fixes for it, which are also includes other bugfixes related to compatibility issues between this two plugins. Also, I've created a hook for line-numbers - in the case, when line highlight is loaded before line numbers. It could be if user manually connects <script> tag of line highlight before line numbers, or if the user has downloaded Prism from the website builder, where line numbers are added after line highlight :) P.S. Sorry for long response |
I can't blame you, can I? ^^ Is the hook required? Not that I'm particularly against it, but since the plugin already runs in the |
|
@Golmote haha :) |
|
Looks good to me! Thanks for working on this! |
Hello.
First of all, sorry about created compatibility issues with this plugins. But 2 years passed after that PR.. I've just missed this point :)
In this PR:
data-startattribute todata-line-numbers-start- because line highlight plugin is using this attribute as wellI've decided to not separate bug fix to into its own branch because it is tied up to each other.