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

[ENH] A simple {tableofcontents} directive #553

Merged
merged 11 commits into from May 15, 2020

Conversation

AakashGfude
Copy link
Member

Have transferred the PR here from MyST-NB executablebooks/MyST-NB#167 along with a few implementations of suggestions by @choldgraf

Syntax :-

```{tableofcontents}

At present it lays out the sub pages of the page where it is declared. I think we might also want it to have internal section links of the page as bullet points. Is that what you meant as well with your suggestion @choldgraf here - executablebooks/MyST-NB#167? or just the external sub-pages.

Adding it in the intro.md page gives :-

image

Adding it to /content-types/index.md page gives :-

image

@AakashGfude
Copy link
Member Author

AakashGfude commented May 7, 2020

Possible second phase features :-

  • passing of a relative path to another page as an argument, and if so then show the toctree for that page.

@AakashGfude AakashGfude changed the title A simple {tableofcontents} directive [ENH] A simple {tableofcontents} directive May 7, 2020
@choldgraf
Copy link
Member

choldgraf commented May 12, 2020

@AakashGfude is this ready to go? (maybe try rebasing on master to see if the tests pass afterward? or fix those tests if we're ready to merge)

@AakashGfude
Copy link
Member Author

@choldgraf do the functionalities look good enough to be merged for now? If yes, then I will take out some time, and correct the tests and also push the styles in sphinx-book-theme.
We can add other changes later then.
Note that it does not give the links to the internal sections of the page for now, which I was not sure should be implemented or not in this directive.

@choldgraf
Copy link
Member

I think it's a good first step! Let's get it merged and try it out to see how it feels

@AakashGfude
Copy link
Member Author

AakashGfude commented May 13, 2020

@choldgraf do you know why the test https://circleci.com/gh/executablebooks/jupyter-book/303?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link is failing?
It says No module named 'jupyter_book.directive'. It seems like the error you get when it is using an old installation of jupyter_book locally for tests.

@choldgraf
Copy link
Member

choldgraf commented May 13, 2020

I think the problem could be that you created a module for the directive but you didn’t add an init.py file

@AakashGfude
Copy link
Member Author

Thanks @choldgraf . The tests seem to be passing now.

@choldgraf
Copy link
Member

oops wait a sec - I just realized you didn't add any tests...can you please add a test to make sure that the structure is what we'd expect? I think just a simple pytest-regressions check should be fine.

@AakashGfude
Copy link
Member Author

@choldgraf hmm. for the toctree directive though, I will need a project with toc.yml and a page with the directive used. Should I create it in the examples folder?

@AakashGfude AakashGfude force-pushed the toc-directive branch 2 times, most recently from 13708fb to 3b27f52 Compare May 14, 2020 09:05
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Looking good! A few quick comments

tests/test_tocdirective.py Outdated Show resolved Hide resolved
jupyter_book/directive/toc.py Outdated Show resolved Hide resolved
jupyter_book/toc.py Outdated Show resolved Hide resolved
@choldgraf
Copy link
Member

re-check the toc.py page - the behavior just changed a little bit, mostly so that page titles are only added with a flag and not by default

@AakashGfude AakashGfude force-pushed the toc-directive branch 2 times, most recently from 7013750 to 7402d53 Compare May 15, 2020 02:59
@AakashGfude
Copy link
Member Author

@choldgraf have done the necessary changes. Let me know what you think

@choldgraf
Copy link
Member

it looks great - let's give it a merge and see how the functionality feels! thanks @AakashGfude 🎉

@choldgraf choldgraf merged commit cb48929 into executablebooks:master May 15, 2020
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.

None yet

2 participants