-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add an option to redirect trailing slash urls #74
base: master
Are you sure you want to change the base?
Conversation
This reverts commit a7f511c.
The best approach from a website visitor's perspective judged based on web best practices is to serve a 301 redirect. That would be faster, more conventional, and still preserve fragments. However, for static site hosts that do not support serving custom 301 redirects (e.g. github pages), this is a reasonable alternative. This PR serves a 200 page
Which uses javascript to preserve the url fragment. If the javascript fails, then there is both an automatic and manual fallback. Both fallbacks fail to preserve the url fragment. This PR is tested at LilithHafner/Chairmarks.jl#80 |
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.
Haven't been able to fully review, but this looks promising and it seems it's already working! Will look into this more properly tomorrow.
# reason, the meta http-equiv will proc, dropping fragments | ||
# If that, too fails, there's an ordinary, human readable | ||
# relative link. | ||
# | ||
# This uses a relative canonical link which is bad form, but | ||
# oh well. We don't have access to the full URL until deploy |
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.
We may have access to the deploy URL through settings.deploy_url
and deploy_decision
, so in that situation we could add that in if it's better.
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.
A recall trying that and finding that field to be empty.
Thanks! I will add a couple of small tests, and merge this before the next release! |
Bump |
Also
It's still functionally untested.
Fixes #64