Table of Contents block: fix links when in archive loop or when using "Plain" permalink structure. #29394
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZebulanStanphill
added
[Type] Bug
3 tasks
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
ZebulanStanphill
force-pushed
the
fix/table-of-contents-links-in-archive
branch
2 times, most recently
from
7a32b71
to
b9ba53a
Compare
|
Thanks @ZebulanStanphill ! Looking forward to the feature. |
|
Hi Zeb @ZebulanStanphill . |
|
@paaljoachim Good idea. I've added testing instructions to PR description. |
|
Hey Zeb. Using WordPress 5.7 RC2. Switched permalinks to plain links. Heading-Page-Breaks.mp4Nicely done. This works fine! --> Sidetrack. |
paaljoachim
approved these changes
Mar 3, 2021
|
@paaljoachim To be clear, you should also test to see if the links still work when viewing the post from another page, e.g. the home page's default blog feed. (Some themes will use HTML-stripped summaries, but Twenty Fourteen, like most of the older default themes, displays the entirety of the posts on the home page.) Or maybe you already tested that, but it wasn't clear from your comment. (I already tested it, and it worked, but it's always nice to get confirmation.)
This is a pretty common issue caused by |
ZebulanStanphill
added
the
Needs Technical Feedback
Offtopic; There's a CSS property for this: https://www.caniuse.com/mdn-css_properties_scroll-margin-top |
gwwar
reviewed
Mar 6, 2021
There was a problem hiding this comment.
Almost! I'm seeing a 301 redirect when a post does not have multiple parts. Everything else tests very well.
The url creation here is a good candidate for unit tests to document the cases for permalink choice and multipart posts. I'm fine if it's a follow up though since it might take some work to set up.
redirect.mp4
This also fixes links for posts using the default "Plain" permalink structure.
ZebulanStanphill
force-pushed
the
fix/table-of-contents-links-in-archive
branch
from
b9ba53a
to
5e84309
Compare
Yeah, unit tests would be neat. I'll try and create some in another PR if no one else wants to. |
gwwar
approved these changes
Mar 6, 2021
There was a problem hiding this comment.
Nice work ✨ I verified that this works well for multipart and single page posts, and am no longer seeing the 301.
Yeah, unit tests would be neat. I'll try and create some in another PR if no one else wants to.
That'd be lovely to follow up on @ZebulanStanphill
ZebulanStanphill
removed
the
Needs Technical Feedback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR fixes two bugs with the Table of Contents block:
https://example.com/?p=7/./2#id, which isn't a valid URL. (The proper one would behttps://example.com/?p=7&page=2#id.) This is fixed by switching over to using thepagequery param to choose which page to go to. This should work with any permalink structure, since on sites with slug-based permalink structures,https://example.com/post-slug?page=2#idwill automatically redirect tohttps://example.com/post-slug/2#id.Special thanks to @asafm7 for pointing out the first bug, which led me to discover the second while trying to fix it.
Testing instructions
YOUR_SITE/wp-admin/options-permalink.php.Checklist: