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

Allow at-contents to take Depth as a UnitRange #1125

Closed
wants to merge 2 commits into from

Conversation

NHDaly
Copy link
Contributor

@NHDaly NHDaly commented Sep 8, 2019

Allow ```@contents to take Depth as a UnitRange:

```@contents
Pages = ["index.md"]
Depth = 2:5
```

This is useful to allow skipping the title of a page (header level 1) when generating a Table of Contents, since the title is often redundant.

Fixes #245

@mortenpi
Copy link
Member

mortenpi commented Sep 8, 2019

At least in the HTML output, Documenter still seems to write them out on the same level. E.g. with Depth = 2:4, the level 2 headings, that should now be on the highest level, still end up on the second level:

contents

I reckon we'd want them to end up on the top level. Not 100% sure at the moment where the issue is coming from though.

This keeps the list starting at the top-level bullets, even if e.g.
`Depth = 2:5` starts only at the second-level headers
@NHDaly
Copy link
Contributor Author

NHDaly commented Sep 10, 2019

I reckon we'd want them to end up on the top level. Not 100% sure at the moment where the issue is coming from though.

I wasn't sure if moving them up a level was desirable or not. But now that you point it out, yeah I think you're right!

After a bit of digging, it looks like the change needs to be made in the renderers, e.g. here:

level = Utilities.header_level(header)

I've just pushed up a commit that subtracts the starting depth from the level. Let me know if that's a clean way to do it, or if there's a different factoring of the code you'd prefer? :) Thanks!

(Also, i'm not really sure how to write unit tests for this... is that something we should do? Thanks)

Screen Shot 2019-09-10 at 11 42 11 AM

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Sorry for forgetting to review this. The solution looks fine, except for a minor bug.

Regarding unit tests: yeah, it's hard to test these things. Using it in the test/example builds is sufficient I'd say.

@@ -1238,7 +1238,7 @@ function domify(ctx, navnode, contents::Documents.ContentsNode)
header = anchor.object
url = string(path, '#', anchor.id, '-', anchor.nth)
node = a[:href=>url](mdconvert(header.text; droplinks=true))
level = Utilities.header_level(header)
level = Utilities.header_level(header) - contents_header_level_offset(contents)
Copy link
Member

Choose a reason for hiding this comment

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

This currently errors with

ERROR: LoadError: UndefVarError: contents_header_level_offset not defined

Just needs a using for it, as the definition is in the Documenter.Documents module.

@mortenpi
Copy link
Member

What happens when you have

* Foo
  - Foo 1
  - Foo 2
* Bar
  - Bar 1
  - Bar 2

and you have e.g. Depth = 2:3? It would just become:

* Foo 1
* Foo 2
* Bar 1
* Bar 2

?

@NHDaly
Copy link
Contributor Author

NHDaly commented Oct 2, 2019

Yeah, that's true...

Maybe what would be better would be to have an option to drop all preceding headers? That is, to only start the table-of-contents from the current position?

So this:

# My Page

## Table of Contents
```@contents
Pages = ["index.md"]
OnlyIndexBelow = true
```

# Section 1
## Subheader
...
# Section 2
...

Would become:

My Page

Table of Contents

  • Section 1
    • Subheader
  • Section 2

Section 1

Subheader

Section 2

...

What do you think about that? The option name needs a bit of work, OnlyIndexBelow = true could be SkipPrevious = true, OnlyIndexFollowing = true, StartHere = true, NoPrevious = true, StartIndex = "Section 1", or something else?

@mortenpi
Copy link
Member

mortenpi commented Oct 6, 2019

In general, you should not have h1 headings on the page anyway (other than the top one), so that behavior would not be an issue. We could treat the Depth option as being quite a low-level feature (i.e. it does a very specific thing and it is up to the user to make sure that it makes sense within their docs).

That is not to say that we couldn't have a fancier option as well for the use case that you have in mind. But you would still need to figure out how to deal with the h1 headings -- they shouldn't be there, but they can be.

Just a few thoughts on the second option:

  • I would maybe have it be a special mode of at-contents, which would would enable with something like
```@contents
PageContents = true
```

or

```@contents
mode = :PageContents
```

It would then automatically figure out which page it is on, so no need to Pages. I guess Depth you could still maybe use?

  • To handle h1s: one possible behavior would be to, if there are any h1 headings below the at-contents block, treat them as h2 and emit a warning. So, in that respect, this mode of at-contents would be opinionated about how you organize your headings.

But all this is quite a bit more work to implement I think. So happy to just go with generalizing the Depth option if that works for your use case.

@mortenpi mortenpi removed this from the 0.24.0 milestone Nov 6, 2019
@NHDaly
Copy link
Contributor Author

NHDaly commented Feb 17, 2022

closing this stale PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use UnitRange for Depth in @contents block
2 participants