Add missing GFM syntax highlighting background #4053

Merged
merged 2 commits into from Oct 26, 2015

Conversation

Projects
None yet
5 participants
@sparanoid
Contributor

sparanoid commented Oct 25, 2015

No description provided.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 25, 2015

Member

Can you show a before and after example, please? Curious what this fixes and how we haven't caught it before. Thanks!

Member

parkr commented Oct 25, 2015

Can you show a before and after example, please? Curious what this fixes and how we haven't caught it before. Thanks!

@sparanoid

This comment has been minimized.

Show comment
Hide comment
@sparanoid

sparanoid Oct 26, 2015

Contributor

@parkr sorry for the lack of reproduction steps:

~ $ mkdir jekyll-rouge && cd jekyll-rouge
~/jekyll-rouge $ jekyll new

Add the following snippets in _posts/2015-10-26-welcome-to-jekyll.markdown:

```ruby
def print_hi(name)
  puts "Hi, #{name}"
end
print_hi('Tom')
#=> prints 'Hi, Tom' to STDOUT.
```

{% highlight ruby %}
def print_hi(name)
  puts "Hi, #{name}"
end
print_hi('Tom')
#=> prints 'Hi, Tom' to STDOUT.
{% endhighlight %}

Then:

~/jekyll-rouge $ jekyll s

Result:

image

I've re-apply background color for .highlight in .highlighter-rouge, this should fix this issue.

edited: simplified steps #4053 (comment)

Contributor

sparanoid commented Oct 26, 2015

@parkr sorry for the lack of reproduction steps:

~ $ mkdir jekyll-rouge && cd jekyll-rouge
~/jekyll-rouge $ jekyll new

Add the following snippets in _posts/2015-10-26-welcome-to-jekyll.markdown:

```ruby
def print_hi(name)
  puts "Hi, #{name}"
end
print_hi('Tom')
#=> prints 'Hi, Tom' to STDOUT.
```

{% highlight ruby %}
def print_hi(name)
  puts "Hi, #{name}"
end
print_hi('Tom')
#=> prints 'Hi, Tom' to STDOUT.
{% endhighlight %}

Then:

~/jekyll-rouge $ jekyll s

Result:

image

I've re-apply background color for .highlight in .highlighter-rouge, this should fix this issue.

edited: simplified steps #4053 (comment)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 26, 2015

Contributor

I'm gonna be honest, there should be no disparity between Rogue and Pygments 👎

Contributor

envygeeks commented Oct 26, 2015

I'm gonna be honest, there should be no disparity between Rogue and Pygments 👎

@sparanoid

This comment has been minimized.

Show comment
Hide comment
@sparanoid

sparanoid Oct 26, 2015

Contributor

@envygeeks just rechecked the config after I found this issue, you don't need to change the config since rouge is the default highlighter, so this PR should be “Add missing GFM syntax highlighting background”

Contributor

sparanoid commented Oct 26, 2015

@envygeeks just rechecked the config after I found this issue, you don't need to change the config since rouge is the default highlighter, so this PR should be “Add missing GFM syntax highlighting background”

@sparanoid sparanoid changed the title from Add missing highlighting CSS class for Rouge to Add missing GFM syntax highlighting background Oct 26, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 26, 2015

Contributor

That's not relevant, it wasn't there for Pygments (apparently -- according to the CSS -- and btw, Pygments is still available, the default is in no way the defacto -- at least in this case.) That means there will be a disparity.

Contributor

envygeeks commented Oct 26, 2015

That's not relevant, it wasn't there for Pygments (apparently -- according to the CSS -- and btw, Pygments is still available, the default is in no way the defacto -- at least in this case.) That means there will be a disparity.

@AndorChen

This comment has been minimized.

Show comment
Hide comment
@AndorChen

AndorChen Oct 26, 2015

@envygeeks As far as I know, there are no much differences between Pygments and Rouge, the problem issued here is caused by kramdown, as you can see in the source code where the .highlighter-rouge class added.

@envygeeks As far as I know, there are no much differences between Pygments and Rouge, the problem issued here is caused by kramdown, as you can see in the source code where the .highlighter-rouge class added.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 26, 2015

Member

Yeah, it looks like Kramdown does this. https://github.com/gettalong/kramdown/blob/7a7bd675b9d2593ad40c26fc4c77bf8407b70b42/lib/kramdown/converter/html.rb#L365-L368

I don't see a huge issue with doing this in the CSS. I wouldn't want to change our Ruby for it, but adding this class seems pretty harmless to me.

Member

parkr commented Oct 26, 2015

Yeah, it looks like Kramdown does this. https://github.com/gettalong/kramdown/blob/7a7bd675b9d2593ad40c26fc4c77bf8407b70b42/lib/kramdown/converter/html.rb#L365-L368

I don't see a huge issue with doing this in the CSS. I wouldn't want to change our Ruby for it, but adding this class seems pretty harmless to me.

parkr added a commit that referenced this pull request Oct 26, 2015

@parkr parkr merged commit e9e4a5d into jekyll:master Oct 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Oct 26, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 26, 2015

Contributor

@parkr, it's not about adding the class, it's that the class is only added to Rogue, unless this affects pygments too, it's a disparity and an oddity to say the least. We should keep it uniform across both... but I didn't look at the CSS fully to even see if Pygments is supported by default in that CSS anyways.

Contributor

envygeeks commented Oct 26, 2015

@parkr, it's not about adding the class, it's that the class is only added to Rogue, unless this affects pygments too, it's a disparity and an oddity to say the least. We should keep it uniform across both... but I didn't look at the CSS fully to even see if Pygments is supported by default in that CSS anyways.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 26, 2015

Member

but I didn't look at the CSS fully to even see if Pygments is supported by default in that CSS anyways.

Yeah, the highlighting CSS was written to the output of Pygments some time ago.

Member

parkr commented Oct 26, 2015

but I didn't look at the CSS fully to even see if Pygments is supported by default in that CSS anyways.

Yeah, the highlighting CSS was written to the output of Pygments some time ago.

sparanoid added a commit to sparanoid/almace-scaffolding that referenced this pull request Jan 19, 2017

feat(style): update syntax highlighter styles
BREAKING CHANGE: I've remove Redcarpet config to enforce using Rouge as default highlighter, it's also recommended to switch to Rouge over Redcarpet as your default highlighter.

More work still need to be done to simplify the stylesheets, ref jekyll/jekyll#4053

@sparanoid sparanoid deleted the sparanoid:patch-1 branch Jan 31, 2017

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.