Re-implement content pages ToC with nesting #418
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the table of contents (ToC) functionality by moving ToC generation from save-time to render-time, implements proper nesting support for h2/h3 headings, and removes the table_of_contents database field in favor of a computed property.
Changes:
- Converted table_of_contents from a stored database field to a computed property
- Implemented nested ToC generation logic that properly handles h2/h3 heading hierarchies and orphaned h3 elements
- Updated SCSS to support nested list styling with proper indentation and icon rendering using mask-image
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/core/migrations/0012_remove_contentpage_table_of_contents.py | Removes the table_of_contents field from ContentPage model |
| apps/core/models/content.py | Replaces stored field with property and implements nested ToC generation logic |
| apps/core/tests/test_content_page.py | Adds comprehensive tests for ToC generation including nesting and edge cases |
| apps/frontend/static_src/scss/components/toc.scss | Updates styles to support nested lists and changes icon rendering approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @property | ||
| def table_of_contents(self): | ||
| return create_table_of_contents(self.body) |
There was a problem hiding this comment.
Converting table_of_contents from a stored field to a property means the ToC will be computed on every access. This involves rendering the StreamField body to HTML and parsing it with BeautifulSoup, which could have performance implications if the property is accessed multiple times per request or if pages have large bodies. Consider adding caching (e.g., using @cached_property) to avoid recomputing the ToC multiple times during a single request.
|
|
||
| self.assertIn('<li><a href="#introduction">Introduction</a>', toc) | ||
| self.assertIn('<ul><li><a href="#details">Details</a></li></ul>', toc) | ||
| self.assertIn('<li><a href="#conclusion">Conclusion</a>', toc) |
There was a problem hiding this comment.
The test expects 'Conclusion' to appear without a closing </li> tag, which seems incomplete. Looking at the ToC generation logic, when an h2 is the last heading, the closing </li> tag is added in the cleanup section (line 66). The assertion should verify the complete structure including the closing tag, or the test should check the full expected HTML to ensure proper tag closure.
| toc = create_table_of_contents(body_html) | ||
|
|
||
| self.assertIn('<li><a href="#orphaned">Orphaned</a></li>', toc) | ||
| self.assertIn('<li><a href="#main">Main</a>', toc) |
There was a problem hiding this comment.
The test expects 'Main' to appear without a closing </li> tag, which seems incomplete. This assertion should verify the complete structure including closing tags. Consider using assertEqual with the full expected HTML output rather than assertIn with partial HTML to ensure the entire structure is correct.
| def test_create_table_of_contents_orphaned_h3(self): | ||
| body_html = """ | ||
| <h3>Orphaned</h3> | ||
| <h2>Main</h2> | ||
| """ | ||
| toc = create_table_of_contents(body_html) | ||
|
|
||
| self.assertIn('<li><a href="#orphaned">Orphaned</a></li>', toc) | ||
| self.assertIn('<li><a href="#main">Main</a>', toc) |
There was a problem hiding this comment.
The test for orphaned h3 headings should verify the complete HTML structure to ensure all tags are properly closed. Consider adding a full assertEqual check for the expected output: '<ul><li><a href="#orphaned">Orphaned</a></li><li><a href="#main">Main</a></li></ul>' to ensure the ToC is well-formed HTML.
Fixes #377, and moves the creation of the ToC from saves, to rendering of the pages. Also adding more tests for ToC creation.