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

Generate Table of Contents #75

Merged
merged 3 commits into from
Jul 31, 2013

Conversation

simeonwillbanks
Copy link
Contributor

In TableOfContentsFilter, generate a Table of Contents as a HTML unordered list of links to headers.

  • Add test cases for directly using HTML::Pipeline::TableOfContentsFilter
  • Add test cases for using HTML::Pipeline::TableOfContentsFilter within a HTML::Pipeline
  • Initialize result[:toc] to an empty string in TableOfContentsFilter#call
  • Initialize @toc to an empty string in TableOfContentsFilter#call
  • Set toc as an attr_reader on TableOfContentsFilter
  • Extract adding header anchor to TableOfContentsFilter#add_header_anchor
  • Define TableOfContentsFilter#append_toc for adding a toc item
  • Define TableOfContentsFilter#finalize_toc for wrapping toc items in a container
  • Update TomDoc
  • Update README.md explaining generated Table of Contents
>> require 'html/pipeline'
=> true
>> TocPipeline =
?>   HTML::Pipeline.new [
?>    HTML::Pipeline::TableOfContentsFilter
>>   ]
=> #<HTML::Pipeline:0x007fbd371eb508 @instrumentation_service=nil, @default_context={}, @filters=[HTML::Pipeline::TableOfContentsFilter], @result_class=Hash>
>> orig = %(<h1>Ice cube</h1><p>is not for the pop chart</p>)
=> "<h1>Ice cube</h1><p>is not for the pop chart</p>"
>> result = {}
=> {}
>> TocPipeline.call(orig, {}, result)
=> {:toc=>"<ul class=\"section-nav\">\n<li><a href=\"#ice-cube\">Ice cube</a></li>\n</ul>", :output=>#<Nokogiri::HTML::DocumentFragment:0x3fde9b900538 name="#document-fragment" children=[#<Nokogiri::XML::Element:0x3fde9b9002b8 name="h1" children=[#<Nokogiri::XML::Element:0x3fde9b9093b8 name="a" attributes=[#<Nokogiri::XML::Attr:0x3fde9b909390 name="name" value="ice-cube">, #<Nokogiri::XML::Attr:0x3fde9b90937c name="class" value="anchor">, #<Nokogiri::XML::Attr:0x3fde9b909368 name="href" value="#ice-cube">] children=[#<Nokogiri::XML::Element:0x3fde9b9082c4 name="span" attributes=[#<Nokogiri::XML::Attr:0x3fde9b908260 name="class" value="octicon octicon-link">]>]>, #<Nokogiri::XML::Text:0x3fde9b909520 "Ice cube">]>, #<Nokogiri::XML::Element:0x3fde9b900254 name="p" children=[#<Nokogiri::XML::Text:0x3fde9b90d864 "is not for the pop chart">]>]>}
>> result[:toc]
=> "<ul class=\"section-nav\">\n<li><a href=\"#ice-cube\">Ice cube</a></li>\n</ul>"
>> result[:output].to_s
=> "<h1>\n<a name=\"ice-cube\" class=\"anchor\" href=\"#ice-cube\"><span class=\"octicon octicon-link\"></span></a>Ice cube</h1><p>is not for the pop chart</p>"
❯ be rake
/Users/simeon/.rbenv/versions/1.9.3-p327-perf/bin/ruby -I"lib:test" -I"/Users/simeon/.rbenv/versions/1.9.3-p327-perf/lib/ruby/gems/1.9.1/gems/rake-10.1.0/lib" "/Users/simeon/.rbenv/versions/1.9.3-p327-perf/lib/ruby/gems/1.9.1/gems/rake-10.1.0/lib/rake/rake_test_loader.rb" "test/html/pipeline/absolute_source_filter_test.rb" "test/html/pipeline/autolink_filter_test.rb" "test/html/pipeline/camo_filter_test.rb" "test/html/pipeline/emoji_filter_test.rb" "test/html/pipeline/image_max_width_filter_test.rb" "test/html/pipeline/markdown_filter_test.rb" "test/html/pipeline/mention_filter_test.rb" "test/html/pipeline/plain_text_input_filter_test.rb" "test/html/pipeline/sanitization_filter_test.rb" "test/html/pipeline/toc_filter_test.rb" "test/html/pipeline_test.rb" 
Run options: 

# Running tests:

......................................................................................

Finished tests in 0.064889s, 1325.3402 tests/s, 2034.2431 assertions/s.

86 tests, 132 assertions, 0 failures, 0 errors, 0 skips

🎵 Thanks to the tests, this Pull Request has a soundtrack. 🎵

…dered list of links to headers.

  * Add test cases for directly using HTML::Pipeline::TableOfContentsFilter
  * Add test cases for using HTML::Pipeline::TableOfContentsFilter within a HTML::Pipeline
  * Initialize result[:toc] to an empty string in TableOfContentsFilter#call
  * Initialize @toc to an empty string in TableOfContentsFilter#call
  * Set toc as an attr_reader on TableOfContentsFilter
  * Extract adding header anchor to TableOfContentsFilter#add_header_anchor
  * Define TableOfContentsFilter#append_toc for adding a toc item
  * Define TableOfContentsFilter#finalize_toc for wrapping toc items in a container
    * Markup from GitHub Styleguide Styling & CSS Navigation 8.3.1
  * Update TomDoc
  * Update README.md explaining generated Table of Contents
@jch
Copy link
Contributor

jch commented Jul 26, 2013

First pull I've gotten with attached soundtrack. We're going to get along, you and I. 🤘

I'll look over the pull today and come back with notes.

@simeonwillbanks
Copy link
Contributor Author

🎶 For sure. Glad you're enjoying it! 🎶

Thanks for taking the time to review.

class TableOfContentsFilter < Filter
PUNCTUATION_REGEXP = RUBY_VERSION > "1.9" ? /[^\p{Word}\- ]/u : /[^\w\- ]/

# Public: Gets the String value of the Table of Contents.
attr_reader :toc
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick, but I don't think there's any value in exposing a public toc reader. To keep the interface consistent across filters, I think it's better to just have one way of accessing filter outputs - via the result hash. The other change you'd need to make is update append_toc to return the toc rather than assign it to @toc:

result[:toc] = append_toc text, name, uniq

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed on keeping the interface consistent. Removing the public toc reader is a good idea.

If we remove @toc, I think we can remove append_toc.

@jch
Copy link
Contributor

jch commented Jul 26, 2013

Checked it out locally and it works great. I'll merge and cut a release after we discuss the changes above 😉

… removing attr_reader. Also, reduce complexity by removing private methods and keep changes within #call. Finally, implement #build_toc to wrap result[:toc] in an unordered list.
@simeonwillbanks
Copy link
Contributor Author

What do you think of the changes?

Awesome! Thanks for including my contribution.

I'd like to continue helping. Maybe we can listen to WEFUNK and hack on #48?

doc
end

# Private: Wrap Table of Contents list items within a container.
def build_toc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe we should move this logic into #call?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya, I think that's a good idea. It's small and not called by anything else. It's clearer to have it all in #call.

@jch
Copy link
Contributor

jch commented Jul 30, 2013

I'd love to hack on #48. It's not too tricky to do, but we'd need to make sure it CI's properly. I'm in SF right now, but normally in Pasadena. We could work on it remotely, or even meet up half way to hack on it. Your call ;)

@simeonwillbanks
Copy link
Contributor Author

Sweet! Sounds like fun. Thanks for offering options. I'll shoot you an email, so we can work out the details.

jch added a commit that referenced this pull request Jul 31, 2013
@jch jch merged commit c0f78e7 into gjtorikian:master Jul 31, 2013
@jch
Copy link
Contributor

jch commented Jul 31, 2013

🍻

@simeonwillbanks
Copy link
Contributor Author

Awesome! Thanks. 🍻 all around...

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.

2 participants