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

Different philosophy for rel prev/next links based on recommendations from Google #87

Merged
merged 7 commits into from
May 28, 2016

Conversation

tfe
Copy link
Contributor

@tfe tfe commented May 12, 2016

Details here: https://webmasters.googleblog.com/2011/09/pagination-with-relnext-and-relprev.html.

This replaces the per-page prev/next links with general ones that will be output whenever there is a paginator with a previous or next page to show. I like the idea of leaning on the paginator to know when there is a previous or next page to link to.

Also resolves #80.

… from Google.

Details here: https://webmasters.googleblog.com/2011/09/pagination-with-relnext-and-relprev.html.

This replaces the per-page prev/next links with general ones that will be output whenever there is a paginator with a previous or next page to show.
@tfe tfe closed this May 12, 2016
@tfe tfe reopened this May 12, 2016
@pathawks
Copy link
Member

pathawks commented May 12, 2016

Looks like next and prev should only be used for pagination and not stringing every article into a chronological chain. Cool, thanks for catching this 👍

Before this can be merged, we will need to add some tests. Are you able to add some specs to spec/jekyll_seo_tag_spec.rb?

Edit: Sorry for closing. My fat thumb hit the wrong button 😡

@pathawks pathawks closed this May 12, 2016
@pathawks pathawks reopened this May 12, 2016
@pathawks
Copy link
Member

Apparently we had no tests for the old way, since removing it didn't break anything 😶

@tfe
Copy link
Contributor Author

tfe commented May 12, 2016

Yes, that's why I didn't write any for this change...

I'll see if I can find some time for it tomorrow.

@pathawks
Copy link
Member

I would love to merge this. @tfe will you have some time to write some tests?

@tfe
Copy link
Contributor Author

tfe commented May 27, 2016

It took quite a bit of trial-and-error and poking around to figure out how to correctly stub the paginator for testing, but there it is. :)

@tfe
Copy link
Contributor Author

tfe commented May 27, 2016

Oops, stand by.

@tfe
Copy link
Contributor Author

tfe commented May 27, 2016

OK, green now. So I'm sure you thought of this, but this is a breaking change to the existing pagination behavior probably worthy of a version number bump.

@pathawks pathawks added this to the 2.0.0 milestone May 28, 2016
@benbalter
Copy link
Collaborator

this is a breaking change to the existing pagination behavior probably worthy of a version number bump

@tfe could you elaborate a bit? Does it break configuration at all, or just changes the output?

@pathawks
Copy link
Member

@benbalter It only changes the output, not the interface.

@tfe Thinking a bit more about this, I think one could make a strong argument that this is merely a bugfix rather than a breaking change.

That said, I think this will be a big enough change that we need to very clearly communicate what is happening and why. I guess I could go either way as far as major/minor version bump.

@benbalter
Copy link
Collaborator

I agree, we can call this a bug fix, but am fine making this a minor bump if we think it warrants it.

@tfe
Copy link
Contributor Author

tfe commented May 28, 2016

Doesn't matter to me; I have no skin in the game. I just wanted to make sure you and the other maintainers thought about it before hitting merge.

@benbalter
Copy link
Collaborator

LGTM. @pathawks do you think it needs a major bump, or is a minor bump sufficient? (I'd even be fine with calling it a bug fix, as long as we communicate the change in behavior). I can't see how this would break any site, even if it is a change in behavior.

@benbalter benbalter mentioned this pull request May 28, 2016
@pathawks
Copy link
Member

Lets do a minor bump. This is bigger than just a patch release because it changes the behavior in a way we need to be sure to communicate, but no big API changes 👍

@pathawks pathawks removed this from the 2.0.0 milestone May 28, 2016
@benbalter benbalter merged commit 43786da into jekyll:master May 28, 2016
@benbalter
Copy link
Collaborator

Thanks @tfe! Nice work.

@jekyll jekyll locked and limited conversation to collaborators Apr 27, 2019
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.

Prev/next links are more appropriate for pagination?
4 participants