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 a toc for html and latex exporter #2178

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

HaudinFlorence
Copy link

@HaudinFlorence HaudinFlorence commented Sep 16, 2024

Add a toc for html and latex exporter.

This PR fixes #1690.

When working on this PR, an correlated issue was opened in jupyterlab jupyterlab/jupyterlab#16795

@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 3 times, most recently from 20a8394 to 16166f0 Compare September 16, 2024 11:52
@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 2 times, most recently from 6bc676c to f7eb4c4 Compare September 16, 2024 16:38
@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 3 times, most recently from 322c26c to 6b3946a Compare September 17, 2024 14:17
@HaudinFlorence HaudinFlorence marked this pull request as ready for review September 17, 2024 14:17
@HaudinFlorence HaudinFlorence marked this pull request as draft September 17, 2024 14:38
@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 2 times, most recently from d16bad8 to 3fefaae Compare September 17, 2024 15:25
@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 2 times, most recently from 2a3e213 to 5de2600 Compare September 18, 2024 09:56
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Neat! I don't have much to say, it looks good to me. I just have a tiny nitpicky comment about the CLI usage

@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 3 times, most recently from 8b6ea2c to 5807458 Compare October 7, 2024 13:57
@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 2 times, most recently from e4cef00 to cb90485 Compare October 7, 2024 14:09
Comment on lines 269 to 273
if resources is None:
resources = {}
resources.update(
{"tableofcontents": extract_titles_from_markdown_input(markdown_collection)}
)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it would better having this in _init_resources as this is the place where we initialize the resources

Copy link
Author

@HaudinFlorence HaudinFlorence Oct 7, 2024

Choose a reason for hiding this comment

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

Thanks for the comment. This has been moved to _init_resources. But this is probably not inducing the proper checking on resources not being None since it is breaking again the linting CI test with this type of error

nbconvert/exporters/html.py:269: error: Item "None" of "Optional[Dict[str, Any]]" has no attribute "update"  [union-attr]

Copy link
Member

Choose a reason for hiding this comment

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

Instead of using update, you could follow what the rest of the code is doing: resources["tableofcontents"] = extract_titles_from_markdown_input(markdown_collection)

Copy link
Author

@HaudinFlorence HaudinFlorence Oct 8, 2024

Choose a reason for hiding this comment

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

This was initially what was used but without any check that resources was not None.
Introducing the suggested change leads to this error message

nbconvert/exporters/html.py:269: error: Unsupported target for indexed assignment ("Optional[Dict[str, Any]]")  [index]
nbconvert/exporters/html.py:358: error: Name "resources" already defined on line 294  [no-redef]

Maybe the type check in the _init_resources function is not valid.

Copy link
Author

@HaudinFlorence HaudinFlorence Oct 9, 2024

Choose a reason for hiding this comment

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

I have displaced the None check condition back at its initial place, in the HTMLExporter

nbconvert/filters/markdown_mistune.py Outdated Show resolved Hide resolved
nbconvert/filters/markdown_mistune.py Outdated Show resolved Hide resolved
share/templates/lab/base.html.j2 Outdated Show resolved Hide resolved
share/templates/lab/index.html.j2 Outdated Show resolved Hide resolved
@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 3 times, most recently from 5e3d85e to 248ddff Compare October 8, 2024 08:54
Fix index.html.j2 to restore the style of the table of content.
@HaudinFlorence HaudinFlorence changed the title Add a toc for html exporter Add a toc for html and latex exporter Oct 9, 2024
…om_notebook_node, include it in the resources dictionary and use it directly in the index.html.j2 template.
@HaudinFlorence HaudinFlorence force-pushed the add_exporter_for_html_toc branch 2 times, most recently from 9452783 to de2ff4e Compare October 14, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement format:HTML pertains to exporting to the HTML format format:LaTeX pertains to exporting to the LaTeX format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

command line argument for table of content.
2 participants