-
Notifications
You must be signed in to change notification settings - Fork 33
Support hash redirects / heading redirects #73
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
base: master
Are you sure you want to change the base?
Conversation
@pawamoy Love to get your input! |
Hi @reteps, thanks a lot for the PR! I'll try to review this tomorrow 🙂 |
Thank you! I think there may be an implementation bug, as the tests don't pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few things that make this PR a bit hard to review:
- there seem to be formatting changes, could you revert them? (maintain previous line length)
- you have renamed the "redirect maps" variables to "redirect entries", is there a practical reason? if not, could you revert those too?
I am currently unable to see where is the impactful change 😅
Hi @pawamoy, could you point out what portion is formatting changes? I genuinely have added about 130 LoC. As for the redirect entries, there is a helper function explaining its difference from the redirect maps. I added this so that I can keep track of overall and hash based redirects, for each base page. |
Ah, my bad, I was probably looking at a single commit instead of the full diff 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made an initial review. The code looks good, though I didn't try looking at the semantics, whether the change makes sense or if there are better ways, etc. To be honest I don't have a lot of mental bandwidth to invest in MkDocs maintenance, so I'm not sure when I'll be able to provide a more meaningful review. Please bear with me 🙏 Others are welcome to review this PR too 🙂
if (window.location.hash === "{old_anchor}") {{ | ||
location.href = "{new_link}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I wonder if we shouldn't escape the variables properly. Something like ... = {old_anchor!r}
, which I believe would work in Javascript code as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a scenario where this would be useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can anchors contain "
? Or would such characters be encoded in the percent format? If yes and not encoded, such anchors would break this Javascript code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that pretty much all headings in mkdocs go through a slugification process that removes special characters (mkdocs-material docs on this). I have personally never seen a link with this type of character in it, but I can add the escaping if you would like.
Co-authored-by: Timothée Mazzucotelli <[email protected]>
Co-authored-by: Timothée Mazzucotelli <[email protected]>
Co-authored-by: Timothée Mazzucotelli <[email protected]>
Co-authored-by: Timothée Mazzucotelli <[email protected]>
Hi @yarikoptic I can give you push access to my fork if you would like to immediately work on this; I won't have some time for another two weeks. |
I think the code-style nits are now resolved, but there is still a bug because the tests are still failing. If anyone want to help pick this up, I can provide push access to this branch. |
Some of the existing test cases redirect away from an existing page -- and with this addition, this causes those tests to fail. For example, existing_pages = [
"README.md",
] And the test case checks that a file
This doesn't seem like desirable behavior -- should I change the test suite? |
@pawamoy any thoughts? |
Resolves #69
I won't claim this is the cleanest implementation, but it works! I tested it on my own site with these rules:
where
foo.md
doesn't exist, andindex.md
does.