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

Replace liquid highlight tag with backticks #5262

Merged
merged 2 commits into from Aug 26, 2016

Conversation

Projects
None yet
10 participants
@ashmaroli
Member

ashmaroli commented Aug 20, 2016

Replace {% highlight ---- %} and {% endhighlight %} with backticks to keep markdown syntax highlight intact for .md docs within site/_docs folder.

{% highlight --- %} is kept intact for lines code blocks involving optional linenos arg and those args not supported in GFM.

@crispgm

This comment has been minimized.

Show comment
Hide comment
@crispgm

crispgm Aug 20, 2016

Member

I agree with you. But are there any reason why jekyll should provide a highlight tag?

Member

crispgm commented Aug 20, 2016

I agree with you. But are there any reason why jekyll should provide a highlight tag?

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 20, 2016

Member

The highlight tag additionally supports including line no.s not natively present in GFM.
All it takes is to add linenos after the language code.

So its presence is advantageous.

Member

ashmaroli commented Aug 20, 2016

The highlight tag additionally supports including line no.s not natively present in GFM.
All it takes is to add linenos after the language code.

So its presence is advantageous.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 21, 2016

Contributor

The highlight tag is a relic of days gone past. However, this needs some discussion. I am for this as I often browse the documentation from Github directly so this gives me an advantage that others have had when they browse the actual website, but only @parkr and @mattr- (both of whom have been around far longer than I or @alfredxing) can tell us if they are even still necessary considering Github defaults to Kramdown now (I don't remember if Github's main site still uses GFM -- I thought it was only comments that did that) and Kramdown has the same support that Jekyll does for Rouge and Pygments, I think it's a good idea to modernize the site that way and remove old relics... put them in the museum (git history.)

Contributor

envygeeks commented Aug 21, 2016

The highlight tag is a relic of days gone past. However, this needs some discussion. I am for this as I often browse the documentation from Github directly so this gives me an advantage that others have had when they browse the actual website, but only @parkr and @mattr- (both of whom have been around far longer than I or @alfredxing) can tell us if they are even still necessary considering Github defaults to Kramdown now (I don't remember if Github's main site still uses GFM -- I thought it was only comments that did that) and Kramdown has the same support that Jekyll does for Rouge and Pygments, I think it's a good idea to modernize the site that way and remove old relics... put them in the museum (git history.)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 21, 2016

Contributor

I should also /cc @jglovier and @benbalter who could also probably provide some insight too.

Contributor

envygeeks commented Aug 21, 2016

I should also /cc @jglovier and @benbalter who could also probably provide some insight too.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 21, 2016

Member

Back in the day, Jekyll used to support both Textile and Markdown as a markup language to use for pages and posts so the {% highlight %} tag was the original solution to provide markup agnostic syntax highlighting. The Jekyll documentation site was created while we still had support for both markup languages and the markdown backend situation was a bit less settled.

Since we're removed support for Textile and have started defaulting to Kramdown as the markdown generator, the {% highlight %} tag is a bit less useful. At this point, I believe its only useful use case is if you want to add line numbers to your code listing, which isn't supported by GFM's extensions to standard Markdown.

I'm 👍 on this change for the Jekyll site.

LGTM

Member

mattr- commented Aug 21, 2016

Back in the day, Jekyll used to support both Textile and Markdown as a markup language to use for pages and posts so the {% highlight %} tag was the original solution to provide markup agnostic syntax highlighting. The Jekyll documentation site was created while we still had support for both markup languages and the markdown backend situation was a bit less settled.

Since we're removed support for Textile and have started defaulting to Kramdown as the markdown generator, the {% highlight %} tag is a bit less useful. At this point, I believe its only useful use case is if you want to add line numbers to your code listing, which isn't supported by GFM's extensions to standard Markdown.

I'm 👍 on this change for the Jekyll site.

LGTM

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Aug 22, 2016

Contributor

👍 to changing our own documentation, and what we suggest users use in our own documentation.

👎 to removing the highlight tag entirely, as it's still necessary for line numbers and for non-Kramdown markdown engines.

@envygeeks out of curiosity, if you had a question, why did you assign instead of cc?

Contributor

benbalter commented Aug 22, 2016

👍 to changing our own documentation, and what we suggest users use in our own documentation.

👎 to removing the highlight tag entirely, as it's still necessary for line numbers and for non-Kramdown markdown engines.

@envygeeks out of curiosity, if you had a question, why did you assign instead of cc?

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 22, 2016

Member

@benbalter, I've already kept 'highlight' intact for existing codes for linenos; and for the documentation regarding the same.

Member

ashmaroli commented Aug 22, 2016

@benbalter, I've already kept 'highlight' intact for existing codes for linenos; and for the documentation regarding the same.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 23, 2016

Contributor

@benbalter mostly because I wanted to test out the new assignment stuff 😜

Contributor

envygeeks commented Aug 23, 2016

@benbalter mostly because I wanted to test out the new assignment stuff 😜

@benbalter benbalter removed their assignment Aug 23, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 23, 2016

Contributor

LGTM.

Contributor

envygeeks commented Aug 23, 2016

LGTM.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 23, 2016

Contributor

/cc @parkr

Contributor

envygeeks commented Aug 23, 2016

/cc @parkr

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 23, 2016

Member

IIRC this changes the look of the blocks due to some CSS weirdness. Let me give it a try locally.

Member

parkr commented Aug 23, 2016

IIRC this changes the look of the blocks due to some CSS weirdness. Let me give it a try locally.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Aug 23, 2016

Member

@parkr it does not seem to be the case:

highlighter-backticks

Member

DirtyF commented Aug 23, 2016

@parkr it does not seem to be the case:

highlighter-backticks

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 23, 2016

Member

@DirtyF If you look closely, you'll see a margin is off there. Alas, the HTML is wildly different:

jekyllrb.com

screen shot 2016-08-23 at 2 53 38 pm

this PR

screen shot 2016-08-23 at 2 53 30 pm

Member

parkr commented Aug 23, 2016

@DirtyF If you look closely, you'll see a margin is off there. Alas, the HTML is wildly different:

jekyllrb.com

screen shot 2016-08-23 at 2 53 38 pm

this PR

screen shot 2016-08-23 at 2 53 30 pm

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 24, 2016

Member

I've made some edits to the _style.scss to remove the discrepancy mentioned above.
Add to this PR or create another?

\cc @jekyll/documentation

Member

ashmaroli commented Aug 24, 2016

I've made some edits to the _style.scss to remove the discrepancy mentioned above.
Add to this PR or create another?

\cc @jekyll/documentation

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 24, 2016

Contributor

This is not a performance problem.

Contributor

envygeeks commented Aug 24, 2016

This is not a performance problem.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 24, 2016

Member

the edit is simply:

.highlighter-rouge .highlight {
  @extend .highlight;
  margin: 0;
  padding: 10px 0.5em;
}
Member

ashmaroli commented Aug 24, 2016

the edit is simply:

.highlighter-rouge .highlight {
  @extend .highlight;
  margin: 0;
  padding: 10px 0.5em;
}
@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 26, 2016

Member

@ashmaroli just add to this one.

Member

mattr- commented Aug 26, 2016

@ashmaroli just add to this one.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 26, 2016

Contributor

Can we get some screenshots of the new stuff? For approval. Thanks!

Contributor

envygeeks commented Aug 26, 2016

Can we get some screenshots of the new stuff? For approval. Thanks!

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Aug 26, 2016

Member

Screenshots

Current

live

PR

pr

Member

ashmaroli commented Aug 26, 2016

Screenshots

Current

live

PR

pr

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 26, 2016

Member

LGTM.

Member

parkr commented Aug 26, 2016

LGTM.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Aug 26, 2016

Member

LGTM

@jekyllbot: merge +site

Member

mattr- commented Aug 26, 2016

LGTM

@jekyllbot: merge +site

@jekyllbot jekyllbot merged commit 1930e13 into jekyll:master Aug 26, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Approved by @parkr. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Aug 26, 2016

@ashmaroli ashmaroli deleted the ashmaroli:highlighter branch Sep 22, 2016

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