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

Fix back_to_top not displaying when no other footer variables are set #1461

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattxwang
Copy link
Member

This is the minimum necessary change to make back_to_top work when there is no custom footer, last edit timestamp, or GitHub edit link.

Two immediate thoughts. First, we have a pair of variables: back_to_top and back_to_top_text. In my opinion, this seems a bit unnecessary; we could just use a back_to_top, and treat any non-nil/false value as the text. We could make this backwards compatible (i.e. support but deprecate back_to_top_text). Any thoughts here?

Secondly, some of these conditions are weak:

{% if site.back_to_top %}
<p><a href="#top" id="back-to-top">{{ site.back_to_top_text }}</a></p>
{% endif %}

Here, this conditional should also check for back_to_top_text, and presumably, this should "bubble up" to the overall if statement on line 4 (similar things for the gh_* variables - the line 4 condition only checks for gh_edit_link). Is this a reasonable concern/take? If so, I can approach this some time (either in this PR, in another PR, etc.).


After we decide the correct behaviour of back_to_top, I'll add documentation to the "Configuration" section!

Separately, @pdmosses mentioned:

The obvious fix is to always render the footer when site.back_to_top is set. However, it would improve the usability of the back-to-top feature if individual pages could override the site setting (to suppress the link on some short pages, or to show it on some long pages).

Happy to do that too - for organizational purposes, I'll punt that to another PR (that I can merge into the same release).


Fixes #1443.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Back-to-top link not shown
1 participant