Add new TableOfContents Page variable (Markdown only)

[nwidger]

Added TableOfContents field to hugolib.Page struct. New function
getTableOfContents is used in convertMarkdown to set the TableOfContents
field.

Added new test file hugolib/page_toc_test.go with a simple test of the
new functionality.

Pull request for issue #153.

[nwidger]

My only concern with the patch as it stands is that the generated TOC stored in the .TableOfContents page variable links to anchors which don't exist in the normal .Content variable since that is generated without the HTML_TOC flag. Passing HTML_TOC when generating .Content generates a TOC with working links but forces the TOC to appear at the top of the page. I don't know how important being able to customize the placement of the TOC is, but perhaps instead of adding a new page variable we could just introduce a new configuration variable that determines if HTML_TOC is passed in when generating .Content.

[spf13]

I think best thing to do is parse the output and place it in another variable. Does that make sense?

I'll try to spend some time digging into this and understand it better.

Steve Francia
spf13.com
@spf13

On Jan 20, 2014, at 5:38 PM, nwidger notifications@github.com wrote:

My only concern with the patch as it stands is that the generated TOC stored in the .TableOfContents page variable links to anchors which don't exist in the normal .Content variable since that is generated without the HTML_TOC flag. Passing HTML_TOC when generating .Content generates a TOC with working links but forces the TOC to appear at the top of the page. I don't know how important being able to customize the placement of the TOC is, but perhaps instead of adding a new page variable we could just introduce a new configuration variable that determines if HTML_TOC is passed in when generating .Content.

—
Reply to this email directly or view it on GitHub.

[nwidger]

Yes, that's definitely one possibility, although it'd certainly be a bit more fragile if the blackfriday package changes its output. Currently, it generates the TOC wrapped in a nav element:

<nav>
<ul>
<li>
<ul>
<li><a href="#toc_0">Blah blah blah</a></li>
<li><a href="#toc_1">Blah blah blah</a></li>
<li><a href="#toc_2">Blah blah blah</a></li>
</ul></li>
</ul>
</nav>

A simple regexp could certainly be written to rip this out of the output and store it in .TableOfContents.

[spf13]

Working on merging this in as part of a general refactor. Should merge in shortly.

[nwidger]

Awesome!

[spf13]

Merged. If you could checkout head and play with it to give some feedback that would be appreciated.

[nwidger]

Awesome, will do!

On Tue, Jan 28, 2014 at 11:42 PM, Steve Francia notifications@github.comwrote:

Merged. If you could checkout head and play with it to give some feedback
that would be appreciated.

Reply to this email directly or view it on GitHubhttps://github.com//pull/173#issuecomment-33556165
.

[nwidger]

So the whole thing works really well, I think. My only gripe is the extra <ul> layer that the TOC gets wrapped around. Right now, the TOC winds up looking like this:

<nav id="TableOfContents">
  <ul>
    <li>
      <ul>
        <li><a href="#toc_0">Blah blah blah</a>
        <li><a href="#toc_1">Blah blah blah!</a></li>
        <li><a href="#toc_2">Blah blah blah</a></li>
        <li><a href="#toc_3">Blah blah blah</a></li>
        <li><a href="#toc_4">Blah blah blah</a></li>
      </ul>
  </ul>
</nav>

when really I think most people would just want:

<nav id="TableOfContents">
  <ul>
    <li><a href="#toc_0">Blah blah blah</a>
    <li><a href="#toc_1">Blah blah blah!</a></li>
    <li><a href="#toc_2">Blah blah blah</a></li>
    <li><a href="#toc_3">Blah blah blah</a></li>
    <li><a href="#toc_4">Blah blah blah</a></li>
  </ul>
</nav>

See what I mean?

[spf13]

This is straight from BlackFriday. If you use h1 tags it uses that top UL.
I don't think we can change it.

For me the ideal would be to use whatever the largest h used as the base rather than always going off h1.
Perhaps a patch upstream would help.

Steve Francia
spf13.com
@spf13

On Jan 30, 2014, at 8:49 AM, nwidger notifications@github.com wrote:

So the whole thing works really well, I think. My only gripe is the extra

layer that the TOC gets wrapped around. Right now, the TOC winds up looking like this:
• • Blah blah blah
  • Blah blah blah!
  • Blah blah blah
  • Blah blah blah
  • Blah blah blah
when really I think most people would just want:
• Blah blah blah
• Blah blah blah!
• Blah blah blah
• Blah blah blah
• Blah blah blah
See what I mean?

—
Reply to this email directly or view it on GitHub.

[nwidger]

I thought that might be the case but didn't check the source. Thanks for adding the feature, it'll definitely come in handy for me. :)

[nwidger]

Actually it appears there's been a change in the way we use blackfriday. Previously, we simply called blackfriday.MarkdownCommon, defined here:

https://github.com/russross/blackfriday/blob/0c38d23ca25d46a03814024113a488baca6b201c/markdown.go#L227

but now markdownRender calls blackfriday.Markdown, but passes different flags/extensions:

https://github.com/spf13/hugo/blob/master/hugolib/page.go#L587

I am using Markdown tables, which no longer work since we don't pass EXTENSION_TABLES into blackfriday.Markdown. I think we should change markdownRender to use the same flags/extensions as blackfriday.MarkdownCommon.

[spf13]

Agree (mostly). We should include all the ones than enable more features, like tables. 
We should also include skip scripts which isn’t on by default but hinders our users. 

Care to make a PR?

Best,
Steve

-- 
Steve Francia
http://stevefrancia.com
http://spf13.com
http://twitter.com/spf13

From: nwidger nwidger
Reply: spf13/hugo reply@reply.github.com
Date: January 30, 2014 at 10:00:48 AM
To: spf13/hugo hugo@noreply.github.com
Subject:  Re: [hugo] Add new TableOfContents Page variable (Markdown only) (#173)
Actually it appears there's been a change in the way we use blackfriday. Previously, we simply called blackfriday.MarkdownCommon, defined here:

https://github.com/russross/blackfriday/blob/0c38d23ca25d46a03814024113a488baca6b201c/markdown.go#L227

but now markdownRender calls blackfriday.Markdown, but passes different flags/extensions:

https://github.com/spf13/hugo/blob/master/hugolib/page.go#L587

I am using Markdown tables, which no longer work since we don't pass EXTENSION_TABLES into blackfriday.Markdown. I think we should change markdownRender to use the same flags/extensions as blackfriday.MarkdownCommon.

—
Reply to this email directly or view it on GitHub.

[nwidger]

Sure, I'll try to get to that tonight.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.