Clean up action view helper #501

Closed
wants to merge 3 commits into
from

2 participants

@buddhamagnet

Used kaminari a few times, love it!

So by way of contribution reduced some redundancy in the view helper and will do more if time permits.

@yuki24 yuki24 commented on the diff Jan 29, 2014
lib/kaminari/helpers/action_view_extension.rb
else
# If last page, add prev link unless first page.
- output << '<link rel="prev" href="' + url_for(params.merge(param_name => (scope.current_page - 1), :only_path => true)) + '"/>' unless scope.first_page?
+ output << '<link rel="prev" href="' + prv + '"/>' unless scope.first_page?
end
@yuki24
Collaborator
yuki24 added a line comment Jan 29, 2014

Actually, since we already have #next_page and #prev_page methods, we can just do something like:

output = ""

if scope.next_page
  output << '<link rel="next" href="' + url_for(params.merge(param_name => scope.next_page, :only_path => true)) + '"/>'
end

if scope.prev_page
  output << '<link rel="prev" href="' + url_for(params.merge(param_name => scope.prev_page, :only_path => true)) + '"/>'
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@buddhamagnet

Thanks for that - have extracted the logic to assemble the link into another method which looks cleaner to me, I may be wrong! Otherwise I'll just revert to the solution you suggest.

@yuki24
Collaborator

What I meant to say was that we don't have to call url_for 4 times in the method so we don't have to add extra methods here. We still have to call it twice but I don't care about such a small duplicate.

@yuki24 yuki24 added a commit that closed this pull request Jan 31, 2014
@yuki24 yuki24 Refactor #rel_next_prev_link_tags method
closes #501
e07914f
@yuki24 yuki24 closed this in e07914f Jan 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment