-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
improve slashing + base href behavior #842
Conversation
felixroos
commented
Dec 5, 2023
- prefix sidebar links & mdx links with base
- add trailing slashes
- refactor doc links to trailing slashes? #841
- Some relative paths break when site base is not root ("/") #832
+ prefix sidebar links & mdx links with base + add trailing slashes
Add in fixes from my fork to slashocalypse branch
added a few more fixes there is still a problem with the fonts, which are loaded from not sure how to get the base href in there.. might work with an api endpoint |
i guess I'll merge this as is, the font urls can be fixed in a separate PR. all the other problems should be fixed + it will hopefully give algolia a better next crawl |
} else if (url.href.startsWith('/')) { | ||
// any other relative url starting with / | ||
// NOTE: this does strip off serialized queries and fragments | ||
newHref += url.pathname.endsWith('/') ? url.pathname : url.pathname + '/'; |
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.
I noticed today that using url.pathname
here does cause at least one tiny regression since it strips fragments off the ends of relative links (which I didn't think was an issue at first): in this excerpt from docs
You might now be able to see this properly here: [open in REPL](/#YXdhaXQgaW5pdEh5ZHJhKCkKbGV0IHBhdHRlcm4gPSAiMyA0IDUgWzYgN10qMiIKc2hhcGUoSChwYXR0ZXJuKSkub3V0KG8wKQpuKHBhdHRlcm4pLnNjYWxlKCJBOm1pbm9yIikucGlhbm8oKS5yb29tKDEpIA%3D%3D)
the fragment needs to stay. So I think quick rework of this to keep fragment is needed.
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.
Oh darn, it's merged.
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.
oh I haven't noticed that
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.
Hotfixed by #845, thanks!