ctubbsii commented on code in PR #14:
URL: https://github.com/apache/thrift-website/pull/14#discussion_r3102873864
##########
static/css/rouge.css:
##########
Review Comment:
There's probably a better way to include this file, using sass/scss to
include it in the main style.css, rather than add a static css asset, but this
works, too.
##########
_plugins/remote_snippets.rb:
##########
@@ -22,7 +22,7 @@ def initialize(tag_name, text, tokens)
def render(context)
title = context.registers[:site].config['title']
prefix = context.registers[:site].config['gitbox_url']
- url = "#{prefix};a=blob_plain;hb=HEAD;f=#{@path}"
+ url = ENV["THRIFT_DIR"] ? File.new("#{ENV["THRIFT_DIR"]}/#{@path}") :
"#{prefix};a=blob_plain;hb=HEAD;f=#{@path}"
Review Comment:
What's this THRIFT_DIR stuff? Is this something you added in development for
testing? Does it need to stay?
##########
_plugins/remote_snippets.rb:
##########
@@ -42,13 +42,14 @@ def render(context)
end
snippet_type = "snippet"
if @type == "direct"
- content = Kramdown::Document.new(content).to_html
+ content = context.registers[:site]
+ .find_converter_instance(Jekyll::Converters::Markdown)
+ .convert(content.to_s)
snippet_type = "page"
else
content = content.force_encoding("utf-8")
- formatter = Rouge::Formatters::HTMLLegacy.new
- lexer = Rouge::Lexer.find_fancy(@type, content)
- content = formatter.format(lexer.lex(content))
+ formatter = Jekyll::Tags::HighlightBlock.send(:new, "highlight", @type,
Liquid::ParseContext.new)
+ content = formatter.send(:add_code_tag, formatter.send(:render_rouge,
content))
Review Comment:
Calling the Rouge formatter directly is closer to the original code, and
it's a pretty intuitive change. Wrapping with the HTML tags shouldn't really
break things. I'm not sure of the risks for calling the private APIs, so I
don't really know which is better. Calling the Rouge formatter directly is more
intuitive to me, but I'm not really a ruby developer, so I don't really know
which is better. I think either are fine, whichever you think is lower risk and
easier to maintain.
##########
_plugins/remote_snippets.rb:
##########
@@ -42,13 +42,14 @@ def render(context)
end
snippet_type = "snippet"
if @type == "direct"
- content = Kramdown::Document.new(content).to_html
+ content = context.registers[:site]
+ .find_converter_instance(Jekyll::Converters::Markdown)
+ .convert(content.to_s)
Review Comment:
I'm not sure this really changes anything, because the default markdown
converter is Kramdown. But, this probably is better to look up dynamically,
rather than hard-code Kramdown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
