Skip to content
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

Pass parameters and anchors in URL to redirected page #229

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

swheaton
Copy link

@swheaton swheaton commented Nov 21, 2020

Summary

This change allows anchors (#heading) and parameters (?q=a) in a URL to be passed along to the redirected page. The use case for this would be when linking readers to a specific section of a page, and wanting the link to work for either the permalink page or the redirected page. I don't have a use case for query parameters, but it's the same idea so I don't see why not.

Current

If oldPath/index.html
redirects to
http://jekyllrb.com/2014/01/03/redirect-me-plz.html

Then any link containing anchors or parameters is broken, such as the following:
http://jekyllrb.com/oldPath#important-subsection?question=answer
Will redirect to the base page, stripping off the extras:
http://jekyllrb.com/2014/01/03/redirect-me-plz.html

Proposed

I propose that the link:
http://jekyllrb.com/oldPath#important-subsection?question=answer
Should redirect to
http://jekyllrb.com/2014/01/03/redirect-me-plz.html#important-subsection?question=answer

Technical Details

  1. The first script now appends location.hash and location.search to the jump location.
  2. The backup redirect solutions - the refresh meta tag and manual redirect links - are edited in a secondary script to append location.hash and location.search.
  3. If JavaScript is turned off, it will redirect to the base URL without anchors or parameters
  4. I don't think location.hash or location.search can be null or undefined, but just in case, there are null/undefined checks to make them explicitly be "".

@swheaton
Copy link
Author

Bump

@swheaton
Copy link
Author

Bump2. Anyone checking this repo?

@cmcl
Copy link

cmcl commented Jun 15, 2021

Hello, this addition is rather useful. Can we get someone to merge it in?

@swheaton
Copy link
Author

maybe @ashmaroli , @DirtyF , @parkr ?

@dylanirlbeck
Copy link

Can we bump this? @DirtyF I'll just tag you since it seems you've contributed recently.

@ashmaroli
Copy link
Member

Hello everyone, I've taken a look at this.
The patch introduces a significant amount of JS code without test coverage. That is not acceptable.
Why not handle the computation in Ruby itself instead of JS?

@swheaton
Copy link
Author

Thank you for taking a look @ashmaroli.

This was months ago but if I remember right, it's because you want to be able to redirect to specific anchors on the permanent page, which happens dynamically. I don't think you can do it in Ruby (which runs at site build time) unless along with registering the redirect you somehow also registered all anchor and parameter possibilities.

At least, I cannot think of a way. But I am no Ruby expert by far. If you have an idea, please let me know and I will be happy to try my hand at an alternative solution.

If not, how do you propose increasing test coverage? The current test framework can only test Ruby code if I'm not mistaken.

@ashmaroli
Copy link
Member

to be able to redirect to specific anchors on the permanent page, which happens dynamically..

Oh right. There's no easy way to this before hand.
We sure are in a sticky situation here..

@eregon
Copy link

eregon commented Jan 28, 2022

Ref: #219

Young-Lord pushed a commit to Young-Lord/jekyll-redirect-from that referenced this pull request Nov 1, 2023
Co-authored-by: Stuart <stuart.wheaton@jhuapl.edu>
Co-authored-by: Chris Buckley <chris@cmbuckley.co.uk>
@swheaton
Copy link
Author

Nothing like a zombie PR! Just need maintainer approval 🤷

@ashmaroli
Copy link
Member

Hello @swheaton, I'll get back to this PR in the coming week. Would it be possible for you to update this branch in sync with the master branch?
You don't have to rebase and force-push. Just a simple git pull upstream master will suffice; (where upstream is the jekyll/jekyll-redirect-from repository).
Thanks.

@swheaton
Copy link
Author

@ashmaroli 🫡

@swheaton
Copy link
Author

Now if you ask me anything substantial about this PR, I don't exactly remember because this was 4 years ago 😝
Happy to try to make any fixes though since folks in the community like this PR apparently.

@ashmaroli
Copy link
Member

Hello again, @swheaton
When you get some time, I would like you to revisit the tests proposed for the feature. From what I see, the only change is adding id="refresh-meta" attribute to previously tested markup which is not the subject of this pull request. To rephrase, I do not see the anchors (and parameters) being tested for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants