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.

Sign up for GitHub

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

Jump to bottom

Issues with multiple references to the same footnote #25

Open
jsma opened this issue Feb 24, 2022 · 5 comments · May be fixed by #40
Open

Issues with multiple references to the same footnote #25

jsma opened this issue Feb 24, 2022 · 5 comments · May be fixed by #40

Comments

@jsma
Copy link
Contributor

jsma commented Feb 24, 2022

The current implementation assigns an ID to <sup> elements based on the footnote's index.

wagtail-footnotes/wagtail_footnotes/blocks.py

Line 47 in 0a13e37

return f'<a href="#footnote-{index}" id="footnote-source-{index}"><sup>[{index}]</sup></a>'

This leads to duplicate id values so that browsers (tested in Chrome and Firefox) will only return to the first occurrence in the copy. This can cause issues if the first instance of "[1]" is not currently visible (e.g. in a collapsed accordion), the browser won't do anything when the "Back to content" link is clicked.

I've started working on an implementation to fix this situation which borrows ideas from Wikipedia. They handle this by always assigning unique IDs and then have individual links back to each reference in the copy, using letters of the alphabet. If there is a single reference, then the caret is clickable, otherwise the caret is not clickable, only the letters are:

Screen Shot 2022-02-24 at 1 10 20 PM

I have a basic WIP in my fork so that the block rendering will handle assigning unique IDs to the links and tracks these on the page object so when footnotes are rendered, they can be accessed.

This WIP does not currently change anything on the front-end. I'm still experimenting but so far can solve our issue by using a custom template and template tag to process multiple back links. For simplicity, I'm just using integers as the link text but it shouldn't be too hard to convert these to letters of the alphabet a la Wikipedia:

{% load core_tags %}
{% load wagtailcore_tags %}

{% if page.footnotes_list %}
  <ol>
    {% for footnote in page.footnotes_list %}
      <li id="footnote-{{ forloop.counter }}">
        {{ footnote.text|richtext }}
        {% with reference_ids=page|get_references:footnote.uuid %}
          {% with num=reference_ids|length %}
            {% if num == 1 %}
              <a href="#{{ reference_ids.0 }}" aria-label="Back to content">↩</a>
            {% else %}
              <span aria-label="Back to content">↩</span>
              {% for reference_id in reference_ids %}
                <a href="#{{ reference_id }}">
                  <sup>
                    <i><b>{{ forloop.counter }}</b></i>
                  </sup>
                </a>
              {% endfor %}
            {% endif %}
          {% endwith %}
        {% endwith %}
      </li>
    {% endfor %}
  </ol>
{% endif %}

And a simple template tag since we can't do variable lookups in dictionaries:

@register.filter
def get_references(value, footnote_uuid):
    return value.footnotes_references[footnote_uuid]

Let me know if something like this could be considered for inclusion. Happy to make a formal and comprehensive PR, but looking for feedback first. Thanks!
@nimasmi
Copy link

nimasmi commented Feb 25, 2022

Thanks @jsma, this looks like a worthwhile addition. In particular, the use of duplicate IDs is a concern, so regardless of the additional features, the fact that this prevents editors from inadvertently creating invalid HTML is a good thing.

I haven't seen I'd be concerned about the multiple calls to the |get_references filter, and wonder whether that can be moved outside the forloop, and converted to something that returns an iterable. Ah, I've just noticed you provide example code for that filter. A dict lookup could work fine if it's precomputed on the page.

@jsma
Copy link
Contributor Author

jsma commented Feb 25, 2022

A dict lookup could work fine if it's precomputed on the page.

Can you clarify? I haven't had my morning tea yet ;) The template tag was just a quick workaround for the limitation of the Django template language where you can't do a dictionary lookup using an attribute of another object e.g. page.footnotes_references.footnote.uuid.

@nimasmi
Copy link

nimasmi commented Feb 25, 2022 •
edited
Loading

Yes, sorry. I was instinctively wary at first when I saw your |get_references filter function, because I had imagined it performing queries every time it is called.

If it's just looking up items from a dict, which has previously been assembled e.g. when instantiating or .serve()ing the page, or similar, then it's fine. You haven't provided the example code for how value.footnotes_references is assembled, so I was just preemptively making sure it doesn't become an expensive, slow function.

As a worse example, a bad implementation might be if value.footnotes_references were a property method, which performed a query, and returned a dict (i.e. not precomputed by the page or the block).

Like I said, you haven't given any example implementation, so I'm sorry I doubted you, but better safe than sorry.

@jsma
Copy link
Contributor Author

jsma commented Feb 25, 2022

Ah, sorry if the link wasn't noticeable in my initial post but the implementation is here in my fork: https://github.com/GreenLightGo/wagtail-footnotes/pull/1/files

@jsma jsma linked a pull request Sep 8, 2022 that will close this issue
Open
@jsma
Copy link
Contributor Author

jsma commented Oct 18, 2022

My previous comment about how Wikipedia handles this was too English-specific. For Chinese, Russian, and several other languages, they use integers rather than letters of the alphabet. This may be the best/only language-neutral option:

Screen Shot 2022-10-18 at 1 21 43 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow multiple references to the same footnote jsma/wagtail-footnotes
2 participants
@jsma @nimasmi