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

Generate redirect_to content for collections #46

Merged
merged 7 commits into from
Sep 13, 2014

Conversation

gjtorikian
Copy link
Member

This should close #45, when the tests pass. Looks like redirect_to pages are not being generated for collections.

/cc @parkr Will probably necessitate another (minor) bump.

@gjtorikian
Copy link
Member Author

Yeah, no matter what, it seems like self.content = self.output is not being set, which renders redirect_to broken.

@parkr
Copy link
Member

parkr commented Sep 13, 2014

That self stuff is on the redirect page, not on the document, so it should be fine. Maybe it's not getting the correct URL or isn't accessing the data properly?

@parkr
Copy link
Member

parkr commented Sep 13, 2014

Is site.read run before these tests run? Or site.process? I forget.

@parkr
Copy link
Member

parkr commented Sep 13, 2014

It looks like doc is nil.

def setup_doc
@site.collections["articles"].docs.first
def setup_doc(doc_filename)
@site.collections["articles"].docs.find { |d| d.path.match(doc_filename) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use relative_path here

@gjtorikian
Copy link
Member Author

I've just pushed up a change to sort of demonstrate what I mean.

With redirect_to, we want the same URL as the document, just different contents; the contents should be a redirect out to some other URL. With a page or a post, this appears to be fine, we've been testing that all along.

But with a collection, somehow, the item's original contents remain. Right now, "a" fix for this is to overwrite item.content = item.output to be the RedirectPage information. This does not seem ideal, though. I don't know why collections are being processed differently. It would be nice if I could just "remove" the document from site.docs_to_write and stick to continuing with site.pages << redirect_page, but nothing I tried seemed to work.

@gjtorikian
Copy link
Member Author

Guh, maybe this is impossible. The "HTML" that gets generated looks like this:

<p>&lt;!DOCTYPE html&gt;
&lt;meta charset=utf-8&gt;</p>
<title>Redirecting...</title>
<p>&lt;link rel=canonical href=”http://www.zombo.com”&gt;
&lt;meta http-equiv=refresh content=”0; url=http://www.zombo.com”&gt;</p>
<h1>Redirecting...</h1>
<p><a href="http://www.zombo.com">Click here if you are not redirected.</a>
<script>location='http://www.zombo.com'</script></p>

The smart curly quotes are also bothering Ruby 1.9.3.

@parkr
Copy link
Member

parkr commented Sep 13, 2014

For the redirect_to file, make it an HTML page instead of a MD page.

@gjtorikian gjtorikian force-pushed the test-redirect-to-on-collections branch from 80e281e to 56ac244 Compare September 13, 2014 01:39
@gjtorikian
Copy link
Member Author

For the redirect_to class, make it an HTML page instead of a MD page.

:finnadie: Of course! Thanks for the hint. Rebased some of my commits above for a cleaner history, and added a note as such to the README about this.

@gjtorikian gjtorikian force-pushed the test-redirect-to-on-collections branch from 56ac244 to 235e3b9 Compare September 13, 2014 01:45
@gjtorikian
Copy link
Member Author

BTW, everything I hate about programming is encapsulated in this PR.

"Oh I'll just do one quick thing this afternoon...."

@gjtorikian gjtorikian force-pushed the test-redirect-to-on-collections branch from 235e3b9 to ec4a352 Compare September 13, 2014 01:54
@parkr
Copy link
Member

parkr commented Sep 13, 2014

BTW, everything I hate about programming is encapsulated in this PR.

I'm 😦 but also 😆 so hard. ❤️ for sticking through it.

This is looking really good to me. What do you think?

@gjtorikian
Copy link
Member Author

If it looks good to you that's great for me. I don't think this needs another security review, so you can probably just update the PR on pages-gem, and I'll merge and release.

@parkr parkr merged commit 3fa6105 into jekyll:master Sep 13, 2014
@gjtorikian gjtorikian deleted the test-redirect-to-on-collections branch September 13, 2014 02:19
parkr added a commit that referenced this pull request Sep 13, 2014
@parkr
Copy link
Member

parkr commented Sep 13, 2014

LET'S DO THIS THANG

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wonky problem with collections?
3 participants