Skip to content
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

Added basic page range navigation. #2579

Merged
merged 11 commits into from Dec 7, 2016

Conversation

@felixfontein
Copy link
Contributor

commented Dec 5, 2016

Fixes #2299.

This PR provides a basic page range navigation as asked for in #2299 (I hope). Needs better styling (both CSS and template).

@Kwpolska

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

Ah, cool. I'll try that.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

Works great! (Except that there are no more ellipses, which can be annoying if you have a large amount of pages in your index.)

@Kwpolska

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

Why not? You could just have an ellipsis that links to # in the Bootstrap pager as well. (I’d prefer … instead, more pages displayed, and you may use raw Unicode characters. We mandate UTF-8 everywhere.)

…set surrounding size).
@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

Now the ellipses are back, and the surrounding size can be modified easier (and is by default 5 instead of 3).

<a href="${page_links[i]}">${i+1}</a>
% endif
% elif i == current_page - surrounding - 1 or i == current_page + surrounding + 1:
<span class="ellipsis">&#x22EF;</span>

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Dec 6, 2016

Member

here, too

This comment has been minimized.

Copy link
@felixfontein

felixfontein Dec 6, 2016

Author Contributor

Fixed.

@@ -2374,11 +2376,16 @@ def generic_index_renderer(self, lang, posts, indexes_title, template_name, cont
lists.append(posts[:kw["index_display_post_count"]])
posts = posts[kw["index_display_post_count"]:]
num_pages = len(lists)
displayed_page_numbers = [utils.get_displayed_page_number(i, num_pages, self) for i in range(max(num_pages, 1))]

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Dec 6, 2016

Member

For a simpler comparison: num_pages or 1

This comment has been minimized.

Copy link
@felixfontein

felixfontein Dec 6, 2016

Author Contributor

Ok.

@@ -2374,11 +2376,16 @@ def generic_index_renderer(self, lang, posts, indexes_title, template_name, cont
lists.append(posts[:kw["index_display_post_count"]])
posts = posts[kw["index_display_post_count"]:]
num_pages = len(lists)
displayed_page_numbers = [utils.get_displayed_page_number(i, num_pages, self) for i in range(max(num_pages, 1))]
page_links = [page_link(i, displayed_page_numbers[i], num_pages, False) for i in range(max(num_pages, 1))]

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Dec 6, 2016

Member
page_links = [page_link(i, page_number, num_pages, False) for i, page_number in enumerate(displayed_page_numbers)]

This comment has been minimized.

Copy link
@felixfontein

felixfontein Dec 6, 2016

Author Contributor

Fixed.

displayed_page_numbers = [utils.get_displayed_page_number(i, num_pages, self) for i in range(max(num_pages, 1))]
page_links = [page_link(i, displayed_page_numbers[i], num_pages, False) for i in range(max(num_pages, 1))]
if kw['show_index_page_navigation']:
temp_map = {page_number - 1: link for page_number, link in zip(displayed_page_numbers, page_links)}

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Dec 6, 2016

Member

Why is this a dict? I’d expect the lists to be sorted already, so this could be a list. In any case, avoid iterating over range(num_pages) more than once — not only you might have missed the case when num_pages = 0, but also [i[1] for i in sorted(temp_map.values())] has more relation to the dict.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Dec 6, 2016

Author Contributor

The list is not sorted already: in case INDEXES_STATIC == True, displayed_page_numbers will be [num_pages, 1, 2, 3, ..., num_pages - 1]. In case INDEXES_STATIC == False, it will be [1, 2, ..., num_pages] (see utils.get_displayed_page_number). To avoid having to duplicate that kind of strange logic in here, I'm using the map.

In case num_pages == 0, page_links_context should have length 0 (and not length 1 as displayed_page_numbers and page_links). This isn't the case for [i[1] for i in sorted(temp_map.values())], which will have length 1.

Anyway, in case num_pages == 0, the whole thing is not working that well anyway. In case INDEXES_PRETTY_PAGE_URL is True, a redirect to a non-existing page will be created, and in any case, the main index page is not created. How about fixing that here as well?

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Dec 7, 2016

Member

If you want to fix this, go ahead. As it stands, this code LGTM, although this comment should appear in the source code to explain the temp_map’s existence.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Dec 7, 2016

Author Contributor

I added the comment. I'll fix that maybe later in a different PR.

felixfontein added 3 commits Dec 6, 2016
@Kwpolska Kwpolska merged commit 2038779 into master Dec 7, 2016
0 of 5 checks passed
0 of 5 checks passed
codacy/pr Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@Kwpolska Kwpolska deleted the page-range-navigation branch Dec 7, 2016
@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2016

Thanks for merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.