Fix Pagination #1198

Merged
merged 8 commits into from Jun 23, 2013

Projects

None yet

3 participants

@parkr
Member
parkr commented Jun 9, 2013

New: Look for an index.html page starting in the paginate_path and searching upward (in the FS tree) until an index.html is found.

  • paginate in subdirectories properly
  • traverse hierarchy (vertically) to find proper index.html file
  • add tests for new cases
  • fix tests for internals that were changed
@parkr
Member
parkr commented Jun 9, 2013

@mattr- this is ready for review :)

@mattr- mattr- commented on the diff Jun 10, 2013
lib/jekyll/configuration.rb
@@ -29,7 +29,7 @@ class Configuration < Hash
'baseurl' => '/',
'include' => ['.htaccess'],
'exclude' => [],
- 'paginate_path' => 'page:num',
+ 'paginate_path' => '/page:num',
@mattr-
mattr- Jun 10, 2013 Member

I'm missing some context for this change. How is the added slash significant here?

@parkr
parkr Jun 10, 2013 Member

It's not significant as far as I know.

@parkr
parkr Jun 10, 2013 Member

I was just testing with both :)

@mattr- mattr- commented on the diff Jun 10, 2013
lib/jekyll/generators/pagination.rb
@@ -45,6 +50,32 @@ def paginate(site, page)
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)
@mattr-
mattr- Jun 10, 2013 Member

Do we have a URL helpers module or something where this method could go instead? I don't think this method belongs on the Pagination class.

@parkr
parkr Jun 22, 2013 Member

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.

@parkr
parkr Jun 23, 2013 Member

What do you think about this? Do you think a URL Helper to cover these two things is a good idea?

@mattr-
mattr- Jun 25, 2013 Member

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.

@mattr- mattr- commented on the diff Jun 10, 2013
lib/jekyll/generators/pagination.rb
+ #
+ # Returns the url of the template page
+ def self.first_page_url(site)
+ 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)
@mattr-
mattr- Jun 10, 2013 Member

hmm, after seeing this method, I'm wondering if we don't need a TemplatePage object for both this method and the previous method. Thoughts?

@mattr- mattr- commented on the diff Jun 10, 2013
lib/jekyll/generators/pagination.rb
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}"
@mattr-
mattr- Jun 10, 2013 Member

Why use a range here?

@parkr
parkr Jun 10, 2013 Member

Compatibility with 1.8.7. In 1.8.7, string[index] returns the ordinal instead of the character.

@mattr- mattr- commented on the diff Jun 10, 2013
test/test_pager.rb
@@ -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))
@mattr-
mattr- Jun 10, 2013 Member

This passes but I don't understand why.

Shouldn't we end up with /blog/index.html here?

@parkr
parkr Jun 10, 2013 Member

No! There's no index.html file in the /blog folder so it'll fall back to the root index.html.

@parkr
Member
parkr commented Jun 22, 2013

@mattr- – Any further comments? I think I'd like to hold off on the URL Helper for a different PR.

@mattr-
Member
mattr- commented Jun 23, 2013

I'm cool with that. :shipit: 👍

On Sat, Jun 22, 2013 at 10:08 AM, Parker Moore notifications@github.com
wrote:

@mattr- – Any further comments? I think I'd like to hold off on the URL Helper for a different PR.

Reply to this email directly or view it on GitHub:
#1198 (comment)

@parkr parkr merged commit f9d1739 into master Jun 23, 2013

1 check passed

default The Travis CI build passed
Details
@parkr parkr deleted the fix-pagination branch Jun 23, 2013
@parkr parkr added a commit that referenced this pull request Jun 23, 2013
@parkr parkr Update history to reflect merge of #1198. 84113c3
@wakproductions

This change seems to break the pagination plugin in Octopress. Using Octopress 2.5, its pagination plugin takes two arguments for pagination_enabled? When I do a $ jekyll build it gives me an error: Generating... /Users/wkotzan/Google Drive/Development/sites/winstonkotzancom/plugins/pagination.rb:72:in `pagination_enabled?': wrong number of arguments (1 for 2) (ArgumentError)

Member

Yeah, 2.5 isn't ready for prime-time.

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