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

Add base class for filterable class-based view #2855

Merged
merged 6 commits into from
Apr 27, 2022
Merged

Conversation

joemull
Copy link
Member

@joemull joemull commented Apr 21, 2022

Here is the work in progress for the CBV base class. I've put it on core and chosen a url and template name ('article_list`) that will work well with the default naming conventions of CBVs in later versions of Django.

I also implemented pagination via a template element, forming the template so it works with existing CSS.

What else should be included at this stage, before we create more specialized views?

@joemull joemull requested review from ajrbyers and mauromsl April 21, 2022 13:37
@joemull joemull marked this pull request as draft April 21, 2022 13:39
@joemull joemull marked this pull request as ready for review April 25, 2022 09:25
Comment on lines 3 to 18
<div class="pagination-block">
<ul class="pagination">
{% if page_obj.has_previous %}
<li class="arrow"><a href="{{ request.path }}?page={{ page_obj.previous_page_number }}">&laquo;</a></li>
{% endif %}

{% for page in page_obj|slice_pages:3 %}
<li class="{% if page_obj.number == page.number %}current{% endif %}">
<a href="?page={{ page.number }}">{{ page.number }}</a>
</li>
{% endfor %}

{% if page_obj.has_next %}
<li class="arrow"><a href="{{ request.path }}?page={{ page_obj.next_page_number }}">&raquo;</a></li>
{% endif %}
</ul>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a usefull common element. One other thing I would add is a button for first and last page, which you can workout with article_list.num_pages.

In order to make it more re-usable, I would use a name like paginated_results and then include this element above with:
{% include "common/elements/pagination.html" with paginated_results=article_list%}

Copy link
Member Author

@joemull joemull Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mauromsl

I've now added the button for first and last via a new template tag, slice_pages_with_first_last_ellipsis.

As for reusability, I'm not clear on how paginated_results that would make it more reusable. I don't refer to article_list in this template, just page_obj (and with the other commit this morning, is_paginated and paginate_by, all three of which are now passed by the CBV base class.

Maybe related--should this be reusable beyond CBVs?

Copy link
Member

@mauromsl mauromsl Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your new template tag, my previous comment is irrelevant. I also like the use of paginator rather than the queryset for consistency with what slice_pages used to do. Well done!

@joemull joemull requested a review from mauromsl April 26, 2022 10:55
@mauromsl mauromsl merged commit 3f18195 into master Apr 27, 2022
@mauromsl mauromsl deleted the 2844-CBV-base-class branch April 27, 2022 07:29
@joemull joemull linked an issue May 12, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add base class for filtered class-based views
2 participants