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
3 times, most recently
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 |
| "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.
wagtail_footnotes/blocks.py
Outdated
|
|
||
| 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 not 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
wagtail_footnotes/blocks.py
Outdated
| # 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
|
No worries. Per #40 (comment) I started working on this a couple weeks back (unfortunately not a simple rebase). I'm up against several professional and personal deadlines at the moment but hope to finish it up later next week. |
jsma
force-pushed
the
support-multiple-references-to-footnotes
branch
from
6990138 to
5a94412
Compare
| html = super().render(value, context=context) | ||
| return self.replace_footnote_tags(value, html, context=context) | ||
|
|
||
| def render_basic(self, value, context=None): |
There was a problem hiding this comment.
I removed render_basic because super.render() will already return this, and at the end of the day, all that is happening is regex substitutions on the output, which is handled in render() above.
| footnote_uuid = match.group(1) | ||
| try: | ||
| index = self.process_footnote(match.group(1), page) | ||
| except (KeyError, ValidationError): |
There was a problem hiding this comment.
I don't see anything that would raise a ValidationError, so I removed it.
|
I've finally rebased this on main and incorporated the template-based rendering changes introduced in #70. I reworked the implementation a bit to avoid the goofy template tag I relied on previously. It now patches the footnotes themselves before appending them to the page, so the footnote will have knowledge of how many times it has been referenced in the text above. I didn't have any bright ideas beyond just tracking 1-indexed integers so that at least footnote_reference.html and footnotes.html would agree on what the |
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
5a94412 to
1bedf7c
Compare
|
Thank you for keeping this up to date @jsma. I aim to review and fully test this in the next two weeks (and sorry it has taken so long!) |
2 participants
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).