-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
correct calculation for number of lines #385
Conversation
A final \n in a <pre> is not rendered by browsers, so we shouldn't include it in the calculation for the number of lines.
Thanks! Why don't we just remove it then, by replacing it with Btw, have you tested a good variety of browsers? |
Tested and working in the latest versions of Firefox, Chrome, Safari, IE, and Android Chrome. Without this change, the way the number of lines is calculated doesn't correspond to how HTML works. It makes more sense to me to fix it where that calculation happens like this patch does, rather than relying on something elsewhere to strip off a newline. |
Is there anything more required to get this merged? |
Wondering about this one as well; any reason why it hasn't been merged in? |
Here's the simplest minimal example in JSFiddle: http://jsfiddle.net/g8zwu9fr/ (which works perfectly well with the suggested patch but adds an extra newline without it) Note that there's a use case for it since e.g. it's exactly what Jekyll (Kramdown) outputs when you have a fenced code block like this:
|
Couldn't it be handled the same way an initial line feed is? See https://github.com/PrismJS/prism/blob/gh-pages/components/prism-core.js#L185 |
Why not just use this patch as is? Rather than trying to hack around the issue, it just does the appropriate thing and fixes the line number calculation to match browsers. |
I'm just trying to find the best way to solve it. |
@@ -5,10 +5,10 @@ Prism.hooks.add('after-highlight', function (env) { | |||
return; | |||
} | |||
|
|||
var linesNum = (1 + env.code.split('\n').length); | |||
var linesNum = env.code.match(/\n(?!$)/g).length + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: could you please re-add the final ;
?
@Golmote The way we're handling the initial line feed is terrible (sorry, I think it's my fault) and is causing problems in dabblet currently (try pressing enter in the beginning of your code). I think we should remove that. As for the last line break, the issue is that browsers don't display that last line, so we shouldn't calculate it. This is not the same as the initial line break, which is displayed (which is why I was "conveniently" removing it). |
@LeaVerou: Thank you for the feedback on this. I might create a plugin to remove the initial line feed then, so that those who want that behaviour can have it. |
A final \n in a
<pre>
is not rendered by browsers, so we shouldn'tinclude it in the calculation for the number of lines.
Fixes #382.