Skip to content
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

remark-prismjs: remove extra newline #4804

Closed
wants to merge 1 commit into from
Closed

remark-prismjs: remove extra newline #4804

wants to merge 1 commit into from

Conversation

docwhat
Copy link
Contributor

@docwhat docwhat commented Apr 2, 2018

Fixes #4802

@gatsbybot
Copy link
Collaborator

gatsbybot commented Apr 2, 2018

Deploy preview for using-drupal ready!

Built with commit 10c389d

https://deploy-preview-4804--using-drupal.netlify.com

The plugin was adding a newline to the end of code snippets for no good
reason.

Fixes #4802
const maxCodeSplits = codeSplits.length - 1
codeSplits.forEach((split, idx) => {
highlightedCode += split.code
if (!split.highlighted && idx < maxCodeSplits) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change — if I'm reading this right, you changed it so that only the last split that's not highlighted has a new line character added?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the issue just we're adding a code split to InlineCode blocks and we never should do that? Is there some way of detecting if we're highlighting an InlineCode block?

@docwhat
Copy link
Contributor Author

docwhat commented Apr 3, 2018

The previous code added newly new lines to all non-highlighted code. It was an attempt to reverse the split on new lines.

The problem is it added an extra newline at the end.

This fixes it.

@KyleAMathews
Copy link
Contributor

What was wrong with the previous pattern? The bug in #4802 only emerged with the inlinecode highlighting changes. This PR just stops adding back new lines altogether — which could be an acceptable fix.

@docwhat
Copy link
Contributor Author

docwhat commented Apr 3, 2018

What was wrong with the previous pattern?

The bug in #4802 only emerged with the inlinecode highlighting changes.

Correct. That's because the previous code didn't pass <code> innerText through highlightCode().

This PR just stops adding back new lines altogether — which could be an acceptable fix.

It doesn't stop all newlines. It prevents an extra newline being added to the end.

The previous code did (roughly) this for the no-highlighted-lines case:

const codeSplits = highlightedCode.split(`\n`)

highlightedCode = ``
codeSplits.forEach(split => {
  highlightedCode += `${split.code}\n`
})

The problem is that this code above adds an additional trailing \n.

That trailing \n was the problem. It was actually a problem before my changes. I had noticed the extra newline and added this to my list of things to fix. I just got to it earlier than I thought. 😃

@docwhat
Copy link
Contributor Author

docwhat commented Apr 3, 2018

FYI: The appveyor check is barfing on an image-sharp thing. I don't think I did that.

@KyleAMathews
Copy link
Contributor

Fixed it in #4823

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants