Fix back_to_top
not displaying when no other footer variables are set
#1461
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
andback_to_top_text
. In my opinion, this seems a bit unnecessary; we could just use aback_to_top
, and treat any non-nil
/false
value as the text. We could make this backwards compatible (i.e. support but deprecateback_to_top_text
). Any thoughts here?Secondly, some of these conditions are weak:
just-the-docs/_includes/components/footer.html
Lines 7 to 9 in a251382
Here, this conditional should also check for
back_to_top_text
, and presumably, this should "bubble up" to the overallif
statement on line 4 (similar things for thegh_*
variables - the line 4 condition only checks forgh_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:
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.