-
Notifications
You must be signed in to change notification settings - Fork 460
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
Conversation
Ah, cool. I'll try that. |
Works great! (Except that there are no more ellipses, which can be annoying if you have a large amount of pages in your index.) |
Why not? You could just have an ellipsis that links to |
…set surrounding size).
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">⋯</span> |
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.
…
here, too
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.
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))] |
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.
For a simpler comparison: num_pages or 1
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.
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))] |
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.
page_links = [page_link(i, page_number, num_pages, False) for i, page_number in enumerate(displayed_page_numbers)]
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.
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)} |
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.
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.
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.
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?
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.
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.
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 added the comment. I'll fix that maybe later in a different PR.
Thanks for merging :) |
Fixes #2299.
This PR provides a basic page range navigation as asked for in #2299 (I hope). Needs better styling (both CSS and template).