Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Issue #310 fixed. Refactored code to use a single method to know if a ta... #354

Closed
wants to merge 1 commit into from

3 participants

@anthonyalberto

Wrote specs for the new methods. Tell me if I missed anything!

@anthonyalberto anthonyalberto Issue #310 fixed. Refactored code to use a single method to know if a…
… tag page should be displayed.

310 - Fixed. Need to write some tests now.

310 - Fixed. Need to write some tests now.

Stubbed Specs

All tests are passing
5d6c1ce
@yuki24 yuki24 was assigned
@zzak
Collaborator

I don't like this change, refactoring a should remove code not add complexity.

@zzak zzak closed this
@yuki24
Collaborator

@zzak I don't think this is actually refactoring, but tweaking how paginate displays gaps/page numbers, and I like the idea. I guess we can do something better, though.

@zzak
Collaborator

I'm not opposed to the idea, just the patch. Please submit a new patch.

@yuki24 yuki24 reopened this
@yuki24
Collaborator

picked up some of this pull request and manually merged 0689b23.

@yuki24 yuki24 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 11, 2013
  1. @anthonyalberto

    Issue #310 fixed. Refactored code to use a single method to know if a…

    anthonyalberto authored
    … tag page should be displayed.
    
    310 - Fixed. Need to write some tests now.
    
    310 - Fixed. Need to write some tests now.
    
    Stubbed Specs
    
    All tests are passing
This page is out of date. Refresh to see the latest.
View
2  app/views/kaminari/_paginator.html.erb 100644 → 100755
@@ -11,7 +11,7 @@
<%= first_page_tag unless current_page.first? %>
<%= prev_page_tag unless current_page.first? %>
<% each_page do |page| -%>
- <% if page.left_outer? || page.right_outer? || page.inside_window? -%>
+ <% if page.display_tag? -%>
<%= page_tag page %>
<% elsif !page.was_truncated? -%>
<%= gap_tag %>
View
2  app/views/kaminari/_paginator.html.haml 100644 → 100755
@@ -10,7 +10,7 @@
= first_page_tag unless current_page.first?
= prev_page_tag unless current_page.first?
- each_page do |page|
- - if page.left_outer? || page.right_outer? || page.inside_window?
+ - if page.display_tag?
= page_tag page
- elsif !page.was_truncated?
= gap_tag
View
2  app/views/kaminari/_paginator.html.slim 100644 → 100755
@@ -11,7 +11,7 @@
== first_page_tag unless current_page.first?
== prev_page_tag unless current_page.first?
- each_page do |page|
- - if page.left_outer? || page.right_outer? || page.inside_window?
+ - if page.display_tag?
== page_tag page
- elsif !page.was_truncated?
== gap_tag
View
20 lib/kaminari/helpers/paginator.rb 100644 → 100755
@@ -150,6 +150,26 @@ def was_truncated?
@last.is_a? Gap
end
+ #Is the page the first one to the right of the left window and the first one to the left of the inner window?
+ def first_page_outside_inner_and_left?
+ (@page == @options[:current_page] - @options[:window] - 1) && (@page == @options[:left] + 1)
+ end
+
+ #Is the page the first one to the left of the right window and the first one to the right of the inner window?
+ def first_page_outside_inner_and_right?
+ (@page == @options[:current_page] + @options[:window] + 1) && (@page == @options[:total_pages] - @options[:right])
+ end
+
+ #The page isn't in any window, but if we truncate it, will we truncate only this page?
+ def avoids_single_truncation?
+ first_page_outside_inner_and_left? || first_page_outside_inner_and_right?
+ end
+
+ #Should we display the link tag?
+ def display_tag?
+ left_outer? || right_outer? || inside_window? || avoids_single_truncation?
+ end
+
def to_i
number
end
View
64 spec/helpers/tags_spec.rb 100644 → 100755
@@ -135,6 +135,70 @@
its(:was_truncated?) { should_not be_true }
end
end
+ describe '#first_page_outside_inner_and_left?' do
+ context 'page == (left + 1) && page == (current_page - window - 1)' do
+ subject { Paginator::PageProxy.new({:current_page => 9, :window => 3, left: 4}, 5, nil) }
+ its(:first_page_outside_inner_and_left?) { should be_true }
+ end
+ context 'page == (left) && page == (current_page - window - 2)' do
+ subject { Paginator::PageProxy.new({:current_page => 9, :window => 3, left: 4}, 4, nil) }
+ its(:first_page_outside_inner_and_left?) { should_not be_true }
+ end
+ context 'page == (left + 2) && page == (current_page - window)' do
+ subject { Paginator::PageProxy.new({:current_page => 9, :window => 3, left: 4}, 6, nil) }
+ its(:first_page_outside_inner_and_left?) { should_not be_true }
+ end
+ end
+ describe '#first_page_outside_inner_and_right?' do
+ context 'page == (total_pages - right) && page == (current_page + window + 1)' do
+ subject { Paginator::PageProxy.new({:total_pages => 17, :current_page => 9, :window => 3, right: 4}, 13, nil) }
+ its(:first_page_outside_inner_and_right?) { should be_true }
+ end
+ context 'page == (total_pages - right + 1) && page == (current_page + window + 2)' do
+ subject { Paginator::PageProxy.new({:total_pages => 17, :current_page => 9, :window => 3, right: 4}, 14, nil) }
+ its(:first_page_outside_inner_and_right?) { should_not be_true }
+ end
+ context 'page == (total_pages - right - 1) && page == (current_page + window)' do
+ subject { Paginator::PageProxy.new({:total_pages => 17, :current_page => 9, :window => 3, right: 4}, 12, nil) }
+ its(:first_page_outside_inner_and_right?) { should_not be_true }
+ end
+ end
+ describe '#avoids_single_truncation?' do
+ context '#first_page_outside_inner_and_left? == true' do
+ subject { Paginator::PageProxy.new({:current_page => 9, :window => 3, left: 4}, 5, nil) }
+ its(:avoids_single_truncation?) { should be_true }
+ end
+ context '#first_page_outside_inner_and_right? == true' do
+ subject { Paginator::PageProxy.new({:total_pages => 17, :current_page => 9, :window => 3, right: 4}, 13, nil) }
+ its(:avoids_single_truncation?) { should be_true }
+ end
+ context '#first_page_outside_inner_and_left? == #first_page_outside_inner_and_right? == false' do
+ subject { Paginator::PageProxy.new({:current_page => 9, :window => 2, left: 4}, 5, nil) }
+ its(:avoids_single_truncation?) { should_not be_true }
+ end
+ end
+ describe '#display_tag?' do
+ context '#left_outer? == true' do
+ subject { Paginator::PageProxy.new({:total_pages => 15, :current_page => 8, :left => 3, :right => 3, :window => 3}, 1, nil) }
+ its(:display_tag?) {should be_true}
+ end
+ context '#right_outer? == true' do
+ subject { Paginator::PageProxy.new({:total_pages => 15, :current_page => 8, :left => 3, :right => 3, :window => 3 }, 15, nil) }
+ its(:display_tag?) { should be_true }
+ end
+ context '#inside_window? == true' do
+ subject { Paginator::PageProxy.new({:total_pages => 15, :current_page => 8, :left => 3, :right => 3, :window => 3 }, 9, nil) }
+ its(:display_tag?) { should be_true }
+ end
+ context '#avoids_single_truncation? == true' do
+ subject { Paginator::PageProxy.new({:total_pages => 15, :current_page => 8, :left => 3, :right => 3, :window => 3 }, 12, nil) }
+ its(:display_tag?) { should be_true }
+ end
+ context 'All of the above is false' do
+ subject { Paginator::PageProxy.new({:total_pages => 15, :current_page => 9, :left => 3, :right => 3, :window => 3 }, 4, nil) }
+ its(:display_tag?) { should_not be_true }
+ end
+ end
end
end
end
Something went wrong with that request. Please try again.