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

Add option to enable toc by default #76

Closed
wants to merge 3 commits into from

Conversation

gandalf3
Copy link

Perhaps it's only me, but I wanted a way to globally enable tables of contents and merely override it on a page-by-page basis. This PR adds an option for that, enable_by_default.

This PR also makes toc_only return nothing when toc is disabled. This might be
a more controversial change (which perhaps should be in a different PR), but I can't really think of a case where the previous behavior would be desired. I also realize this probably breaks some tests. Unfortunately, when I went to look, a whole bunch of tests seemed to be failing even on master. I recognize that it's a bit poor form to submit a PR without updating the tests, but I'm not too sure what's going on there (plus I'm not familiar with ruby at all).

gandalf3 added 3 commits March 14, 2019 01:13
Also make `toc_only` return nothing when toc is disabled. This might be
a more controversial change, but I can't think of a case where
outputting two copies of the entire content would be desired.
@toshimaru
Copy link
Owner

toshimaru commented Mar 19, 2019

You can configure default value on your _config.yml

defaults:
  -
    scope:
      path: "" # an empty string here means all files in the project
    values:
      layout: "default"
      toc: true # This means toc: true by default

ref. https://jekyllrb.com/docs/configuration/front-matter-defaults/


So, I'd say we don't need to implement it in jekyll-toc itself, please use your own _config.yml.

Thanks for sending PR!

@toshimaru toshimaru closed this Mar 19, 2019
@gandalf3
Copy link
Author

gandalf3 commented Mar 20, 2019

Ah, I can't believe I didn't realize that. Thanks :P

It's a minor matter, but what is your opinion on making toc_only return nothing when toc is disabled? If you like, I can submit a new PR with just that change (though it's such a small change it hardly seems necessary).

@toshimaru
Copy link
Owner

toshimaru commented Mar 20, 2019

what is your opinion on making toc_only return nothing when toc is disabled?

This is a good idea. 👍 (I'd say this behavior is a bug 😩 )

I can submit a new PR

Go ahead. :)

I think just a one-line change?

return html unless toc_enabled?

@gandalf3
Copy link
Author

gandalf3 commented Mar 22, 2019

Yep, though again I'm not sure about tests. #77

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