Opened 6 days ago

Last modified 17 hours ago

#36175 assigned Cleanup/optimization

Implement consistent pagination across admin pages.

Reported by: Antoliny Owned by: Antoliny
Component: contrib.admin Version: 5.1
Severity: Normal Keywords: pagination
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no
Pull Requests:19172 build:failure

Description (last modified by Antoliny)

Current pagination implemented in the admin pages performs the same functionality but is classified differently.
(Due to this difference, "Show all" is provided in list_display but not in history_view.)
When applying pagination in the change_list page, it was implemented through template tags.

ChangeList

# change_list.html
...
{% block pagination %}{% pagination cl %}{% endblock %}
...

# templatetags/admin_list.py

def pagination(cl):
    """
    Generate the series of links to the pages in a paginated list.
    """
    pagination_required = (not cl.show_all or not cl.can_show_all) and cl.multi_page
    page_range = cl.paginator.get_elided_page_range(cl.page_num) if pagination_required else []
    need_show_all_link = cl.can_show_all and not cl.show_all and cl.multi_page
    return {
        'cl': cl,
        'pagination_required': pagination_required,
        'show_all_url': need_show_all_link and cl.get_query_string({ALL_VAR: ''}),
        'page_range': page_range,
        'ALL_VAR': ALL_VAR,
        '1': 1,
    }

HistoryView

# object_history.html 
...
    <p class="paginator">
      {% if pagination_required %}
        {% for i in page_range %}
          {% if i == action_list.paginator.ELLIPSIS %}
            {{ action_list.paginator.ELLIPSIS }}
          {% elif i == action_list.number %}
            <span class="this-page">{{ i }}</span>
          {% else %}
            <a href="?{{ page_var }}={{ i }}" {% if i == action_list.paginator.num_pages %} class="end" {% endif %}>{{ i }}</a>
          {% endif %}
        {% endfor %}
      {% endif %}
      {{ action_list.paginator.count }} {% if action_list.paginator.count == 1 %}{% translate "entry" %}{% else %}{% translate "entries" %}{% endif %}
    </p>
...

Of course, I believe implementing this through a template tag would be a better approach. However, the reason history view had to be implemented this way is that the pagination related template tags only work with ChangeList.

Looking towards the future, this structure will require rewriting duplicate logic like object_history.html again.
(+ I think it could be quite painful if we need to extend or customize it.)

I'm confident that it would be better to manage these common parts through a template tag in one place.

According to the ticket's flags, the next step(s) to move this issue forward are:

  • For anyone except the patch author to review the patch using the patch review checklist and either mark the ticket as "Ready for checkin" if everything looks good, or leave comments for improvement and mark the ticket as "Patch needs improvement".

Change History (6)

comment:1 by Antoliny, 6 days ago

Owner: set to Antoliny
Status: newassigned

comment:2 by Antoliny, 6 days ago

Description: modified (diff)

comment:3 by Sarah Boyce, 4 days ago

Summary: Implement consistent paginator across admin pages.Implement consistent pagination across admin pages.
Triage Stage: UnreviewedAccepted

I will close #36174 as a duplicate of this ticket and accept this one, thank you

comment:4 by Antoliny, 19 hours ago

Has patch: set

comment:5 by Antoliny, 19 hours ago

Needs tests: set
Patch needs improvement: set

comment:6 by Antoliny, 17 hours ago

Needs tests: unset
Patch needs improvement: unset
Note: See TracTickets for help on using tickets.
Back to Top