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

{:no_toc} on highest level heading results in unrendered markup #35

Closed
Philzen opened this issue May 15, 2020 · 5 comments · Fixed by #36
Closed

{:no_toc} on highest level heading results in unrendered markup #35

Philzen opened this issue May 15, 2020 · 5 comments · Fixed by #36
Labels
bug Something isn't working

Comments

@Philzen
Copy link

Philzen commented May 15, 2020

My Markdown

## some prominent header i want to hide
{: .no_toc}

### Subheading A

### Subheading B

### Subheading C

TOC Usage

I'm including toc.html in a file called _includes/sidebar.html, which in turn is included in the main _layouts with the standard option:

    {%- include toc.html html=content -%}

Expected TOC

<ul>
      <li><a href="#subheading-a">Subheading A</a></li>
      <li><a href="#subheading-b">Subheading B</a></li>
      <li><a href="#subheading-c">Subheading C</a></li>
</ul>

Actual TOC

<div class="highlighter-rouge">
    <div class="highlight">
        <pre class="highlight">
            <code>
                -  [Subheading A](#subheading-a)
                -  [Subheading B](#subheading-b)
                -  [Subheading C](#subheading-c)
            </code>
        </pre>
    </div>
</div>

Could also reproduce this starting with a first-level header. So the general level does not matter, trying to hide the first (higher-than-the-following) heading makes it break. Also it does not break when hiding the first header when the following ones are on the same level.

Otherwise - thanks for this awesome liquid Kung Fu. It's a real life-saver as the {:toc}-Tag simply won't work for me outside of the main content files, while toc.html does it out-of-the-box 😄

@Philzen Philzen added the bug Something isn't working label May 15, 2020
@allejo
Copy link
Owner

allejo commented May 19, 2020

Otherwise - thanks for this awesome liquid Kung Fu. It's a real life-saver as the {:toc}-Tag simply won't work for me outside of the main content files, while toc.html does it out-of-the-box 😄

You're welcome! Glad this has been able to help you out 😄

Check out to see if #36 fixes this bug for you.

@Philzen
Copy link
Author

Philzen commented May 20, 2020

I can confirm that #36 fixes the bug as i described it - so that's a clear improvement as it doesn't break in such cases anymore.

However it leads to another potential problem or unwanted user scenario. Maybe my fault, as i'm afraid i was too generic in my issue description. The page i actually discovered this problem is structured like

## some prominent header i want to hide
{: .no_toc}

### Subheading A

## some other header that's cool to show

### Subheading B

### Subheading C

So with the fix, the other header (that's cool to show) will be embezzled in the TOC (meaning it only lists 3rd level headings and omits the 2nd level completely) - effectively it's as if i had included it with {%- include toc.html html=content h_max=3 -%}, which i of course haven't 😉. This fix may or may not open up a pandora's box of new user complaints or questions as it may not be an obvious behaviour at first.

My feeling is the standard behaviour should still list all TOC entries and not automagically hide anything on a higher level than the first heading encountered that doesn't have a {: .no_toc} markup. Question remains if "Subheading A" should then be displayed as 2nd level or pushed to 3rd level with a blank 2nd level entry before it (requiring users to hide that 2nd level css disc with custom styles). Maybe do whatever is easiest to implement and mark the "Subheading A" entry with a no-parent class or something similar.

@allejo
Copy link
Owner

allejo commented May 20, 2020

Given your snippet, this is how the following document structure would look like according to the TOC, which really doesn't make sense.

  - Subheading A
- some other header that's cool to show
  - Subheading B
  - Subheading C

To me, it doesn't make sense that Subheading A is "promoted" to a level 2 heading and being the same as the real h2.

I think if any logic were added to handle this situation, it'd be to modify the {: .no_toc} class to exclude any subheadings from the TOC if the parent is not a part of the TOC. Given this behavior, your TOC would render like so,

- some other header that's cool to show
  - Subheading B
  - Subheading C

Completely skip the "Subheading A" section since it doesn't have a parent and I don't feel like it makes sense to promote its level.

@Philzen
Copy link
Author

Philzen commented May 20, 2020

Having rethought this - #36 really is a good merge as it fixes the issue at hand.

I'd redraw my statement on opening up the pandora's box b/c #36 makes :.no_toc behave exactly the same as if the higher-level heading was not there in the first place - so if at all, that was already opened all along.

What we're discussing now is a. basically a different discussion and b. might either lead to a breaking change or otherwise create inconsistent behaviour between having .no_toc set and not having the markdown file start with a highest-level heading.

So i'd be really satisfied seeing #36 go in and leave the aforementioned discussion up to another issue.

@Philzen Philzen closed this as completed May 20, 2020
@allejo
Copy link
Owner

allejo commented May 20, 2020

What we're discussing now is a. basically a different discussion and b. might either lead to a breaking change or otherwise create inconsistent behaviour between having .no_toc set and not having the markdown file start with a highest-level heading.

I agree; this should be left for another issue to get feedback from more users on the behavior and see if anyone even wants that behavior.

In the meanwhile, I've tagged v1.0.12 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants