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
Fix Pagination #1198
Fix Pagination #1198
Changes from all commits
ce3fa7f
a71b755
bd0e45c
81a2d03
6e80ba8
0521871
7dc6767
7f10b32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,13 @@ class Pagination < Generator | |
# | ||
# Returns nothing. | ||
def generate(site) | ||
site.pages.dup.each do |page| | ||
paginate(site, page) if Pager.pagination_enabled?(site.config, page) | ||
if Pager.pagination_enabled?(site) | ||
if template = template_page(site) | ||
paginate(site, template) | ||
else | ||
Jekyll.logger.warn "Pagination:", "Pagination is enabled, but I couldn't find" + | ||
"an index.html page to use as the pagination template. Skipping pagination." | ||
end | ||
end | ||
end | ||
|
||
|
@@ -33,18 +38,44 @@ def paginate(site, page) | |
all_posts = site.site_payload['site']['posts'] | ||
pages = Pager.calculate_pages(all_posts, site.config['paginate'].to_i) | ||
(1..pages).each do |num_page| | ||
pager = Pager.new(site.config, num_page, all_posts, pages) | ||
pager = Pager.new(site, num_page, all_posts, pages) | ||
if num_page > 1 | ||
newpage = Page.new(site, site.source, page.dir, page.name) | ||
newpage.pager = pager | ||
newpage.dir = File.join(page.dir, Pager.paginate_path(site.config, num_page)) | ||
newpage.dir = Pager.paginate_path(site, num_page) | ||
site.pages << newpage | ||
else | ||
page.pager = pager | ||
end | ||
end | ||
end | ||
|
||
# Static: Fetch the URL of the template page. Used to determine the | ||
# path to the first pager in the series. | ||
# | ||
# site - the Jekyll::Site object | ||
# | ||
# Returns the url of the template page | ||
def self.first_page_url(site) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a URL helpers module or something where this method could go instead? I don't think this method belongs on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URL Helpers? Nope. Might be a good idea to add, but I'd rather do that in a different PR. It could also handle generation of URLs for Pages and Posts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about this? Do you think a URL Helper to cover these two things is a good idea? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really see the pattern that I was seeing before so I don't think we need a URL Helper to cover these two things anymore. |
||
if page = Pagination.new.template_page(site) | ||
page.url | ||
else | ||
nil | ||
end | ||
end | ||
|
||
# Public: Find the Jekyll::Page which will act as the pager template | ||
# | ||
# site - the Jekyll::Site object | ||
# | ||
# Returns the Jekyll::Page which will act as the pager template | ||
def template_page(site) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, after seeing this method, I'm wondering if we don't need a |
||
site.pages.dup.select do |page| | ||
Pager.pagination_candidate?(site.config, page) | ||
end.sort do |one, two| | ||
two.path.size <=> one.path.size | ||
end.first | ||
end | ||
end | ||
end | ||
|
||
|
@@ -62,39 +93,78 @@ def self.calculate_pages(all_posts, per_page) | |
(all_posts.size.to_f / per_page.to_i).ceil | ||
end | ||
|
||
# Determine if pagination is enabled for a given file. | ||
# Determine if pagination is enabled the site. | ||
# | ||
# config - The configuration Hash. | ||
# page - The Jekyll::Page with which to paginate | ||
# site - the Jekyll::Site object | ||
# | ||
# Returns true if pagination is enabled, false otherwise. | ||
def self.pagination_enabled?(config, page) | ||
!config['paginate'].nil? && | ||
page.name == 'index.html' && | ||
subdirectories_identical(config['paginate_path'], page.dir) | ||
def self.pagination_enabled?(site) | ||
!site.config['paginate'].nil? && | ||
site.pages.size > 0 | ||
end | ||
|
||
# Static: Determine if a page is a possible candidate to be a template page. | ||
# Page's name must be `index.html` and exist in any of the directories | ||
# between the site source and `paginate_path`. | ||
# | ||
# config - the site configuration hash | ||
# page - the Jekyll::Page about which we're inquiring | ||
# | ||
# Returns true if the | ||
def self.pagination_candidate?(config, page) | ||
page_dir = File.dirname(File.expand_path(remove_leading_slash(page.path), config['source'])) | ||
paginate_path = remove_leading_slash(config['paginate_path']) | ||
paginate_path = File.expand_path(paginate_path, config['source']) | ||
page.name == 'index.html' && | ||
in_hierarchy(config['source'], page_dir, File.dirname(paginate_path)) | ||
end | ||
|
||
# Determine if the subdirectories of the two paths are the same relative to source | ||
# | ||
# paginate_path - the paginate_path configuration setting | ||
# source - the site source | ||
# page_dir - the directory of the Jekyll::Page | ||
# paginate_path - the absolute paginate path (from root of FS) | ||
# | ||
# Returns whether the subdirectories are the same relative to source | ||
def self.subdirectories_identical(paginate_path, page_dir) | ||
File.dirname(paginate_path).gsub(/\A\./, '') == page_dir.gsub(/\/\z/, '') | ||
def self.in_hierarchy(source, page_dir, paginate_path) | ||
return false if paginate_path == File.dirname(paginate_path) | ||
return false if paginate_path == Pathname.new(source).parent | ||
page_dir == paginate_path || | ||
in_hierarchy(source, page_dir, File.dirname(paginate_path)) | ||
end | ||
|
||
# Static: Return the pagination path of the page | ||
# | ||
# site_config - the site config | ||
# site - the Jekyll::Site object | ||
# num_page - the pagination page number | ||
# | ||
# Returns the pagination path as a string | ||
def self.paginate_path(site_config, num_page) | ||
return nil if num_page.nil? || num_page <= 1 | ||
format = site_config['paginate_path'] | ||
def self.paginate_path(site, num_page) | ||
return nil if num_page.nil? | ||
return Generators::Pagination.first_page_url(site) if num_page <= 1 | ||
format = site.config['paginate_path'] | ||
format = format.sub(':num', num_page.to_s) | ||
File.basename(format) | ||
ensure_leading_slash(format) | ||
end | ||
|
||
# Static: Return a String version of the input which has a leading slash. | ||
# If the input already has a forward slash in position zero, it will be | ||
# returned unchanged. | ||
# | ||
# path - a String path | ||
# | ||
# Returns the path with a leading slash | ||
def self.ensure_leading_slash(path) | ||
path[0..0] == "/" ? path : "/#{path}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use a range here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Compatibility with 1.8.7. In 1.8.7, |
||
end | ||
|
||
# Static: Return a String version of the input without a leading slash. | ||
# | ||
# path - a String path | ||
# | ||
# Returns the input without the leading slash | ||
def self.remove_leading_slash(path) | ||
ensure_leading_slash(path)[1..-1] | ||
end | ||
|
||
# Initialize a new Pager. | ||
|
@@ -104,9 +174,9 @@ def self.paginate_path(site_config, num_page) | |
# all_posts - The Array of all the site's Posts. | ||
# num_pages - The Integer number of pages or nil if you'd like the number | ||
# of pages calculated. | ||
def initialize(config, page, all_posts, num_pages = nil) | ||
def initialize(site, page, all_posts, num_pages = nil) | ||
@page = page | ||
@per_page = config['paginate'].to_i | ||
@per_page = site.config['paginate'].to_i | ||
@total_pages = num_pages || Pager.calculate_pages(all_posts, @per_page) | ||
|
||
if @page > @total_pages | ||
|
@@ -119,9 +189,9 @@ def initialize(config, page, all_posts, num_pages = nil) | |
@total_posts = all_posts.size | ||
@posts = all_posts[init..offset] | ||
@previous_page = @page != 1 ? @page - 1 : nil | ||
@previous_page_path = Pager.paginate_path(config, @previous_page) | ||
@previous_page_path = Pager.paginate_path(site, @previous_page) | ||
@next_page = @page != @total_pages ? @page + 1 : nil | ||
@next_page_path = Pager.paginate_path(config, @next_page) | ||
@next_page_path = Pager.paginate_path(site, @next_page) | ||
end | ||
|
||
# Convert this Pager's data to a Hash suitable for use by Liquid. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,17 @@ | |
|
||
class TestPager < Test::Unit::TestCase | ||
|
||
def build_site(config = {}) | ||
base = Jekyll::Configuration::DEFAULTS.deep_merge({ | ||
'source' => source_dir, | ||
'destination' => dest_dir, | ||
'paginate' => 1 | ||
}) | ||
site = Jekyll::Site.new(base.deep_merge(config)) | ||
site.process | ||
site | ||
end | ||
|
||
should "calculate number of pages" do | ||
assert_equal(0, Pager.calculate_pages([], '2')) | ||
assert_equal(1, Pager.calculate_pages([1], '2')) | ||
|
@@ -12,49 +23,32 @@ class TestPager < Test::Unit::TestCase | |
end | ||
|
||
should "determine the pagination path" do | ||
assert_nil(Pager.paginate_path(Jekyll::Configuration::DEFAULTS, 1)) | ||
assert_equal("page2", Pager.paginate_path(Jekyll::Configuration::DEFAULTS, 2)) | ||
assert_nil(Pager.paginate_path(Jekyll::Configuration::DEFAULTS.merge('paginate_path' => '/blog/page-:num'), 1)) | ||
assert_equal("page-2", Pager.paginate_path(Jekyll::Configuration::DEFAULTS.merge('paginate_path' => '/blog/page-:num'), 2)) | ||
assert_equal("/index.html", Pager.paginate_path(build_site, 1)) | ||
assert_equal("/page2", Pager.paginate_path(build_site, 2)) | ||
assert_equal("/index.html", Pager.paginate_path(build_site({'paginate_path' => '/blog/page-:num'}), 1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This passes but I don't understand why. Shouldn't we end up with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No! There's no index.html file in the |
||
assert_equal("/blog/page-2", Pager.paginate_path(build_site({'paginate_path' => '/blog/page-:num'}), 2)) | ||
assert_equal("/index.html", Pager.paginate_path(build_site({'paginate_path' => '/blog/page/:num'}), 1)) | ||
assert_equal("/blog/page/2", Pager.paginate_path(build_site({'paginate_path' => '/blog/page/:num'}), 2)) | ||
assert_equal("/contacts/index.html", Pager.paginate_path(build_site({'paginate_path' => '/contacts/page:num'}), 1)) | ||
assert_equal("/contacts/page2", Pager.paginate_path(build_site({'paginate_path' => '/contacts/page:num'}), 2)) | ||
assert_equal("/contacts/index.html", Pager.paginate_path(build_site({'paginate_path' => '/contacts/page/:num'}), 1)) | ||
assert_equal("/contacts/page/2", Pager.paginate_path(build_site({'paginate_path' => '/contacts/page/:num'}), 2)) | ||
end | ||
|
||
context "pagination disabled" do | ||
setup do | ||
stub(Jekyll).configuration do | ||
Jekyll::Configuration::DEFAULTS.merge({ | ||
'source' => source_dir, | ||
'destination' => dest_dir | ||
}) | ||
end | ||
@config = Jekyll.configuration | ||
end | ||
|
||
should "report that pagination is disabled" do | ||
page = OpenStruct.new({ :name => 'index.html', :dir => '/' }) | ||
assert !Pager.pagination_enabled?(@config, page) | ||
assert !Pager.pagination_enabled?(build_site('paginate' => nil)) | ||
end | ||
|
||
end | ||
|
||
context "pagination enabled for 2" do | ||
setup do | ||
stub(Jekyll).configuration do | ||
Jekyll::Configuration::DEFAULTS.merge({ | ||
'source' => source_dir, | ||
'destination' => dest_dir, | ||
'paginate' => 2 | ||
}) | ||
end | ||
|
||
@config = Jekyll.configuration | ||
@site = Site.new(@config) | ||
@site.process | ||
@site = build_site('paginate' => 2) | ||
@posts = @site.posts | ||
end | ||
|
||
should "report that pagination is enabled" do | ||
page = OpenStruct.new({ :name => 'index.html', :dir => '/' }) | ||
assert Pager.pagination_enabled?(@config, page) | ||
assert Pager.pagination_enabled?(@site) | ||
end | ||
|
||
context "with 4 posts" do | ||
|
@@ -63,23 +57,23 @@ class TestPager < Test::Unit::TestCase | |
end | ||
|
||
should "create first pager" do | ||
pager = Pager.new(@config, 1, @posts) | ||
pager = Pager.new(@site, 1, @posts) | ||
assert_equal(2, pager.posts.size) | ||
assert_equal(2, pager.total_pages) | ||
assert_nil(pager.previous_page) | ||
assert_equal(2, pager.next_page) | ||
end | ||
|
||
should "create second pager" do | ||
pager = Pager.new(@config, 2, @posts) | ||
pager = Pager.new(@site, 2, @posts) | ||
assert_equal(2, pager.posts.size) | ||
assert_equal(2, pager.total_pages) | ||
assert_equal(1, pager.previous_page) | ||
assert_nil(pager.next_page) | ||
end | ||
|
||
should "not create third pager" do | ||
assert_raise(RuntimeError) { Pager.new(@config, 3, @posts) } | ||
assert_raise(RuntimeError) { Pager.new(@site, 3, @posts) } | ||
end | ||
|
||
end | ||
|
@@ -90,31 +84,31 @@ class TestPager < Test::Unit::TestCase | |
end | ||
|
||
should "create first pager" do | ||
pager = Pager.new(@config, 1, @posts) | ||
pager = Pager.new(@site, 1, @posts) | ||
assert_equal(2, pager.posts.size) | ||
assert_equal(3, pager.total_pages) | ||
assert_nil(pager.previous_page) | ||
assert_equal(2, pager.next_page) | ||
end | ||
|
||
should "create second pager" do | ||
pager = Pager.new(@config, 2, @posts) | ||
pager = Pager.new(@site, 2, @posts) | ||
assert_equal(2, pager.posts.size) | ||
assert_equal(3, pager.total_pages) | ||
assert_equal(1, pager.previous_page) | ||
assert_equal(3, pager.next_page) | ||
end | ||
|
||
should "create third pager" do | ||
pager = Pager.new(@config, 3, @posts) | ||
pager = Pager.new(@site, 3, @posts) | ||
assert_equal(1, pager.posts.size) | ||
assert_equal(3, pager.total_pages) | ||
assert_equal(2, pager.previous_page) | ||
assert_nil(pager.next_page) | ||
end | ||
|
||
should "not create fourth pager" do | ||
assert_raise(RuntimeError) { Pager.new(@config, 4, @posts) } | ||
assert_raise(RuntimeError) { Pager.new(@site, 4, @posts) } | ||
end | ||
|
||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing some context for this change. How is the added slash significant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not significant as far as I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just testing with both :)