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

Update Redcarpet support to version 2 #570

Closed
wants to merge 7 commits into from
Closed

Update Redcarpet support to version 2 #570

wants to merge 7 commits into from

Conversation

tombell
Copy link
Contributor

@tombell tombell commented Jun 5, 2012

Updates the version of Redcarpet to version 2. Also uses Albino to highlight the syntaxes in fenced code blocks. It also applies the SmartyPants rendering to everything, this could be changed. Not sure how valuable it would be to add the extra GitHub sugar as functionality.

@mojombo
Copy link
Contributor

mojombo commented Jun 6, 2012

I think code fencing should be enabled by default, and this needs to be rebased on top of the pygments.rb commit to use that.

@tombell
Copy link
Contributor Author

tombell commented Jun 12, 2012

fenced_code_blocks is enabled by default as long as no_fenced_code_blocks isn't specified as an extension. Also added a test to make sure the fenced blocks are being rendered without having to specify the extension.

@trans
Copy link
Contributor

trans commented Jun 17, 2012

+1 Just tried it out and it worked great.

@trans
Copy link
Contributor

trans commented Jun 17, 2012

One minor note, looks like a extraneous space is being prefixed to code blocks that are being syntax highlighted.

@tombell
Copy link
Contributor Author

tombell commented Jun 17, 2012

Example case?

@trans
Copy link
Contributor

trans commented Jun 17, 2012

Soon as I get my new blog up (hopefully later today) I post the links to it and source.

@trans
Copy link
Contributor

trans commented Jun 17, 2012

Hey @tombell. I was looking at the code for this, and I was thinking that the use of anonymous class --Class.new(Redcarpet::Render::HTML), makes it hard to extend via a plugin. Do you think it can create an actual named subclass instead?

@tombell
Copy link
Contributor Author

tombell commented Jun 18, 2012

Possibly.

@trans
Copy link
Contributor

trans commented Jun 18, 2012

Okay, here is an example of the space issue: http://trans.github.com/2012/06/17/kill-the-proxy-and-save-toplevel.html

Notice how the first line of each code example is indented slightly more than the rest. Maybe its from something I am doing, but I don't think so because it started when I switched to redcarpet.

If you want to look at my site's source, it is a little bit tricky b/c I use my site's wiki repo for it. But you can clone it if you want to look at it. I think this should work:

$ git clone git://github.com/trans/trans.github.com.wiki.git

@tombell
Copy link
Contributor Author

tombell commented Jun 18, 2012

I have tested the specific markdown for that post using my own layout and stylesheets.

test

And as you can see it lines up perfectly. This was created using my redcarpet2 branch. So I would believe it is either something with your styles and/or font choice. Also if you highlight the text on your rendered version you will notice that the spaces are actually correct and that the first line is indented slightly, not with a space.

@trans
Copy link
Contributor

trans commented Jun 18, 2012

Ok. I'll see what I can figure out then. Thanks for taking the time to look at it.

Btw, your example has really nice colorization. Could I get a copy of the .css file for it? I would love to use that on my site.

@tombell
Copy link
Contributor Author

tombell commented Jun 18, 2012

Sure, http://tomb.io/css/monokai.css

And the CSS for the actual box

code, pre {
  font-family: Consolas, Menlo, "Courier New", monospace;
  font-size: 14px;
}

pre {
  background-color: #fafafa;
  border: 1px solid #d5d5d5;
  border-left: 10px solid #d5d5d5;
  font-family: Consolas, Menlo, "Courier New", monospace;
  font-size: 14px;
  line-height: 20px;
  margin-bottom: 22px;
  margin-left: -25px;
  overflow-x: auto;
  padding: 11px 15px 12px;
}

pre::-webkit-scrollbar {
  height: 25px;
}

pre::-webkit-scrollbar-button:start,
pre::-webkit-scrollbar-button:end {
  display: none;
}

pre::-webkit-scrollbar-track-piece  {
  background-color: #eee;
}

pre::-webkit-scrollbar-thumb {
  background-color: #bbb;
  border: 7px solid #eee;
  -webkit-background-clip: padding-box;
  -webkit-border-radius: 12px;
}

p code,
li code {
  border: 1px solid #ccc;
  background-color: #fafafa;
  font-size: 14px;
  line-height: 20px;
  padding: 1px 5px;
  white-space: nowrap;
}

@trans
Copy link
Contributor

trans commented Jun 18, 2012

Ah man... Fantastic! And yes, the space turned out to be a CSS blip. So, thanks again!

Now back to our regulatory scheduled program....

@trans trans mentioned this pull request Jun 18, 2012
@trans
Copy link
Contributor

trans commented Jun 18, 2012

Hi again. Seems I've run into a bit of a conundrum. See #581. I was trying to modify redcarpet patch to use {% raw %} by default, but then realized that the liquid template is being processed before the markup. So I don't see any way to do it.

This is the second time I've run into this. I'm starting to wonder if maybe the markup should be handled first, then the liquid template. Of course, that might have it's own problems.

@tombell
Copy link
Contributor Author

tombell commented Jun 18, 2012

I think that is out of the scope of this pull request, and people should probably manually use {% raw %}.

@bjoerge
Copy link

bjoerge commented Jul 11, 2012

Whats the status on this pull req? I'd really like to see this go into the next release.

@@ -88,7 +86,27 @@ def convert(content)
setup
case @config['markdown']
when 'redcarpet'
Redcarpet.new(content, *@redcarpet_extensions).to_html
@renderer ||= Class.new(Redcarpet::Render::HTML) do
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this class be mokeypatched when we require redcarpet instead of every time that the renderer is needed? I see after require 'redcarpet':

require 'redcarpet'
class Redcarpet::Render::HTML
  # monkeypatch here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I remember why now, the require is in the body of a method, and you can't do class definitions in the middle of method bodies.

/Users/tom/.rbenv/versions/1.9.3-p194/lib/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require': /Users/tom/Development/jekyll/lib/jekyll/converters/markdown.rb:17: class definition in method body (SyntaxError)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah scratch, that I've used Class.new ... instead of it's working fine!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right

@isaacsanders
Copy link

@mojombo What needs to happen to this for you to accept?

@tombell
Copy link
Contributor Author

tombell commented Aug 6, 2012

I think @mojombo is pretty busy at the minute, which is why PRs are going pretty slowly.

@tombell tombell mentioned this pull request Aug 7, 2012
@tombell tombell closed this Aug 10, 2012
@sol
Copy link

sol commented Oct 18, 2012

@tombell You closed that issue. Anything wrong with your patch?

@tombell
Copy link
Contributor Author

tombell commented Oct 18, 2012

#619

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants