Allow multiple references to the same footnote #40
Conversation
|
@jsma I know this is quite late, but would you be able to add a test or two for this? |
jsma
force-pushed
the
support-multiple-references-to-footnotes
branch
2 times, most recently
from
d7f9015
to
75ec95f
Compare
Previously every link in the rich text for the same footnote had the same `id` attribute, which breaks the "Back to content" link (or just always returns the user to the first reference). This now generates a unique `id` for each footnote reference and updates the `footnotes.html` template to generate unique links back to each reference in the content.
jsma
force-pushed
the
support-multiple-references-to-footnotes
branch
from
75ec95f
to
6990138
Compare
|
I completely reworked my initial implementation which required some force pushes. I don't know what planet I was on when I first did this but I fixed the Sorry for any previous CI noise. I added Python 3.11 type hints out of habit, which didn't fail when I ran |
| @@ -38,19 +38,47 @@ def setUp(self): | |||
| [ | |||
| { | |||
| "type": "paragraph", | |||
| "value": f'<p>This is a paragraph with a footnote. <footnote id="{uuid}">1</footnote></p>', | |||
| "value": ( | |||
| f'<p>This is a paragraph with a footnote. <footnote id="{uuid}">[{uuid[:6]}]</footnote></p>' | |||
There was a problem hiding this comment.
The [{uuid[:6]}] bit isn't strictly required since FIND_FOOTNOTE_TAG doesn't care what's between <footnote></footnote> but I opted to make it an accurate representation of what is actually stored in the database.
| @@ -37,17 +40,28 @@ def replace_footnote_tags(self, value, html, context=None): | |||
| page = new_context["page"] | |||
| if not hasattr(page, "footnotes_list"): | |||
| page.footnotes_list = [] | |||
There was a problem hiding this comment.
I think we can do away with footnotes_list in favour of the new footnotes_references.
There was a problem hiding this comment.
These are two different things. footnotes_list is the list of the actual Footnote objects to be rendered at the bottom of the rendered page, and footnotes_references is a dict that maps a Footnote's UUID to a list of all of the references found in the rich text blocks, so that jump links back to each individual reference can be rendered.
I didn't have any better idea of how to make the Footnote objects aware of the one or more references contained in the rich text blocks. This is why I made the goofy template tag so I could at least do dict lookups using the UUID in the template to fetch the references.
I'm working on getting this rebased on main now that the custom template PR was merged.
There was a problem hiding this comment.
Right, that makes sense. I think some additional comments would be helpful then, and perhaps some type hinting, then there will be no ambiguity. Thank you
| """ | ||
| if hasattr(value, "footnotes_references"): | ||
| return value.footnotes_references.get(footnote_uuid, []) | ||
| return [] |
There was a problem hiding this comment.
I am no sure we need this. if we drop footnotes_list, we can update the template to check for page.footnotes_references and tweak the loops accordingly
| # 0-based) and if it's the second link to the first footnote, it will be "footnote-source-1-1", etc. | ||
| # This ensures the ids are unique throughout the page and allows for the template to generate links from | ||
| # the footnote back up to the distinct references in the content. | ||
| link_id = f"footnote-source-{index}-{len(page.footnotes_references[footnote_uuid])}" |
There was a problem hiding this comment.
suggestion: keep indexes consistent. for regular users 1-1, 1-2 etc make more sense than 1-0. 1-1
This is an attempt to fix #25
At present, this only ensures that each linked reference will have a unique HTML
idso that the back links on the footnotes will work (by linking to the first reference). This does not yet include UI changes to expose links to each individual reference in the content but there is a proposed approach in #25 (comment).