Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add #pagination_link_tags helper method #324

Open
wants to merge 9 commits into
from

Conversation

Projects
None yet
9 participants

This adds a #rel_next_prev_link_tags method to will_paginate. Essentially
taken straight from Kaminari's implementation

#In head:
<head>
  <title>My Website</title>
  <%= yield :head %>
</head>

#Somewhere in body:
<% content_for :head do %>
  <%= pagination_link_tags @items %>
<% end %>
@lloydmeta lloydmeta Add #rel_next_prev_link_tags helper method
This adds a #rel_next_prev_link_tags method to will_paginate. Essentially
taken straight from [Kaminari's implementation](https://github.com/amatsuda/kaminari/pull/200/files)

    #In head:
    <head>
      <title>My Website</title>
      <%= yield :head %>
    </head>

    #Somewhere in body:
    <% content_for :head do %>
      <%= rel_next_prev_link_tags @items %>
    <% end %>
b4bf6cf

@mislav mislav commented on an outdated diff Jul 16, 2013

lib/will_paginate/view_helpers.rb
+ #
+ # ==== Examples
+ # Basic usage:
+ #
+ # In head:
+ # <head>
+ # <title>My Website</title>
+ # <%= yield :head %>
+ # </head>
+ #
+ # Somewhere in body:
+ # <% content_for :head do %>
+ # <%= rel_next_prev_link_tags @items %>
+ # <% end %>
+ #
+ # #-> <link rel="next" href="/items/page/3" /><link rel="prev" href="/items/page/1" />
@mislav

mislav Jul 16, 2013

Owner

This example doesn't faithfully reflect what would be returned. The generated URLs are absolute, no?

@mislav mislav commented on an outdated diff Jul 16, 2013

lib/will_paginate/view_helpers.rb
+ # Basic usage:
+ #
+ # In head:
+ # <head>
+ # <title>My Website</title>
+ # <%= yield :head %>
+ # </head>
+ #
+ # Somewhere in body:
+ # <% content_for :head do %>
+ # <%= rel_next_prev_link_tags @items %>
+ # <% end %>
+ #
+ # #-> <link rel="next" href="/items/page/3" /><link rel="prev" href="/items/page/1" />
+ #
+ def rel_next_prev_link_tags(collection)
@mislav

mislav Jul 16, 2013

Owner

Not liking this name. How about pagination_link_tags()?

@mislav mislav commented on an outdated diff Jul 16, 2013

lib/will_paginate/view_helpers.rb
+ # <head>
+ # <title>My Website</title>
+ # <%= yield :head %>
+ # </head>
+ #
+ # Somewhere in body:
+ # <% content_for :head do %>
+ # <%= rel_next_prev_link_tags @items %>
+ # <% end %>
+ #
+ # #-> <link rel="next" href="/items/page/3" /><link rel="prev" href="/items/page/1" />
+ #
+ def rel_next_prev_link_tags(collection)
+ current_page = collection.current_page
+ output = ""
+ if current_page == 1
@mislav

mislav Jul 16, 2013

Owner

Too much repetition in this conditional. How about simply 2 conditions:

  1. If there's a previous page, generate link rel=prev
  2. If there's a next page, generate link rel=next

@mislav mislav commented on an outdated diff Jul 16, 2013

spec/view_helpers/action_view_spec.rb
+ class << helper
+ include ActionDispatch::Routing::UrlFor
+ include Routes.url_helpers
+ include WillPaginate::ActionView
+ end
+ helper.default_url_options[:host] = 'example.com'
+ helper.default_url_options[:controller] = 'dummy'
+ helper}
+ let(:collection) { (1..30).to_a }
+ let(:page_one) { collection.paginate(page: 1, per_page: 10)}
+ let(:page_two) { collection.paginate(page: 2, per_page: 10)}
+ let(:page_three) { collection.paginate(page: 3, per_page: 10)}
+
+ context 'the first page' do
+ subject { helper.rel_next_prev_link_tags page_one }
+ it { should be_a String }
@mislav

mislav Jul 16, 2013

Owner

It's useless to test if the result should be a String object. It would be much better to compare it for equality to what we expect:

it { should include('<link rel="next" href="http://example.com/dummy/?page=2" />') }
it { should_not match /rel="prev"/ }
@lloydmeta lloydmeta Modify Link Rel helper method and Specs
1. Correct the example given in the comment to reflect the fact that absolute path is generated
2. Change the method name to be less verbose
3. Update method specs to make them more meaningful
4. Change logic in helper to be more DRY
723d8c8

Agreed on all points and thus updated the PR.

That said, the helper still has 3 branches in the conditional because we still need to handle 3 cases..

@lloydmeta lloydmeta Modify helper to be even more DRY
Finally saw what mislav was saying :P
1d9f70f

Ah, did you mean the way it is now ? 2 conditions :)

@mislav mislav and 1 other commented on an outdated diff Jul 16, 2013

lib/will_paginate/view_helpers.rb
+ #
+ # In head:
+ # <head>
+ # <title>My Website</title>
+ # <%= yield :head %>
+ # </head>
+ #
+ # Somewhere in body:
+ # <% content_for :head do %>
+ # <%= pagination_link_tags @items %>
+ # <% end %>
+ #
+ # #-> <link rel="next" href="http://example.com/items/page/3" /><link rel="prev" href="http://example.com/items/page/1" />
+ #
+ def pagination_link_tags(collection)
+ output = ""
@mislav

mislav Jul 16, 2013

Owner

How about:

      output = []
      link = '<link rel="%s" href="%s" />'
      output << link % ["prev", url_for(:page => collection.previous_page, :only_path => false) if collection.previous_page
      output << link % ["next", url_for(:page => collection.next_page, :only_path => false) if collection.next_page
      output.join("\n").html_safe
@lloydmeta

lloydmeta Jul 16, 2013

Nice :) Updated.

Updated with recommended logic :)

ylin commented Jul 22, 2013

Kaminari's patch contains a call to params.merge, necessary for maintaining existing parameters, such as on a search page. I had to add that in and the code becomes like this:

    output << link % ["prev", url_for(params.merge(:page => collection.previous_page, :only_path => false))] if collection.previous_page
    output << link % ["next", url_for(params.merge(:page => collection.next_page, :only_path => false))] if collection.next_page
@lloydmeta lloydmeta Add support for params passed in to the controller
Since Kaminari supports params passed into the controller when rendering
link rels so that existing parameters (e.g. for searches) are maintained,
we should have that as well.
edb8a20

@ylin Good point. Added support for that as well.

ylin commented Jul 23, 2013

Do you see any security problem with that, the fact that we are stuffing
user passed string into html_safe tags?

On Monday, July 22, 2013, Lloyd wrote:

@ylin https://github.com/ylin Good point. Added support for that as
well.


Reply to this email directly or view it on GitHubhttps://github.com/mislav/will_paginate/pull/324#issuecomment-21389876
.

@ylin That's true (and another good point). Maybe it would be fine if we filtered out everything relating to the options that affect url_for (doc), and additionally sanitize the params (maybe using something like sanitize)?

Suggestions are welcome.

ylin commented Jul 27, 2013

Actually, on a second thought I think it's fine. Since whatever params user
pass in through the URL, it's already processed by Rails once, we are
simply repeating it again in the URL, so it would be safe to process it
again. I think this is good for now.

On Tue, Jul 23, 2013 at 4:14 PM, Lloyd notifications@github.com wrote:

@ylin https://github.com/ylin That's true (and another good point).
Maybe it would be fine if we filtered out everything relating to the
options that affect url_for (dochttp://apidock.com/rails/ActionView/Helpers/UrlHelper/url_for),
and additionally sanitize the params (maybe using something like sanitizehttp://apidock.com/rails/ActionView/Helpers/SanitizeHelper/sanitize),
?

Suggestions are welcome.


Reply to this email directly or view it on GitHubhttps://github.com/mislav/will_paginate/pull/324#issuecomment-21453219
.

@mislav what are your thoughts on the current state of this PR?

Owner

mislav commented Aug 7, 2013

The PR is alright, although I will skip the whitespace changes when merging this. They're distracting.

I'll merge this in for the next major release.

bpruvost commented Nov 4, 2013

Great initiative with this PR.

I've been trying to implement this but in my particular case, there is a bug:
I am using a custom renderer (that extends WillPaginate::ActionView::LinkRenderer and overrides the url method) when calling will_paginate, so the URLs created within this method (with url_for) are different.

Any thoughts on how to fix this? I was thinking that calling myRenderer.url() instead of url_for() could do the trick, but that's apparently not possible because it will go through the "usual" flow when calling super (= LinkRenderer.url) and break when encountering some undeclared variables.

Unfortunately I'm not too familiar with will_paginate's flow so I'm not sure how else to solve this, but if you guys have an idea, I'll be happy to contribute.

@JasonBarnabe JasonBarnabe referenced this pull request in stylish-userstyles/userstyles Mar 11, 2014

Closed

Add link rel next/prev #20

@mislav mislav added the todo label Jun 18, 2014

tielur commented Oct 13, 2014

@mislav any ETA on the next major release that this will go out in?

@mislav has this feature been added yet?

Since I keep getting notifications for people wanting it merged or bumping the related #183 issue, I assume that (some) people do want this feature in will_paginate.

This PR is now a bit over a year and a half years old, and I haven't touched Ruby for about that long, so I'm getting more and more rusty. In fact, Ruby (bundler, or RVM, or ...) doesn't even work properly on my current home computer. It took me a while to figure out why the Travis builds were failing (that Rails 3.2 w/ 2.1.2 is a WTF for sure).

I've solved the merge conflicts in hopes that this will help speed things along before this PR reaches legal drinking age ;) Good luck !

As of writing, this merges and passes all tests (plus a I threw in a bonus: sudo: false in .travis.yml means faster CI.)

@lloydmeta I was hoping @mislav would merge this into a release, but otherwise we intend on taking your code and monkey-patching it into our config. Thanks!

xevix commented Feb 4, 2015

👍

@jonatack jonatack added a commit to jonatack/will_paginate that referenced this pull request Dec 6, 2015

@jonatack jonatack Add #pagination_link_tags helper method
This is a port of PR #324 by @lloydmeta in will_paginate:

mislav#324

in response to issue #183:

'SEO optimize pagination with rel="prev" & rel="next"'

mislav#183
28fa48f

hi @mislav and @lloydmeta thanks for your work on the gem and this particular PR. What is the current status though? I happen to need this. Thanks.

@mpolakis mpolakis added a commit to mpolakis/will_paginate that referenced this pull request Mar 11, 2016

@jonatack @mpolakis jonatack + mpolakis Add #pagination_link_tags helper method
This is a port of PR #324 by @lloydmeta in will_paginate:

mislav#324

in response to issue #183:

'SEO optimize pagination with rel="prev" & rel="next"'

mislav#183
c388e3a

@christemple christemple pushed a commit to notonthehighstreet/will_paginate that referenced this pull request Oct 6, 2016

@jonatack jonatack + Christopher Temple and Geza Kiss Add #pagination_link_tags helper method
This is a port of PR #324 by @lloydmeta in will_paginate:

mislav#324

in response to issue #183:

'SEO optimize pagination with rel="prev" & rel="next"'

mislav#183
fdb95d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment