mdn/django-locallibrary-tutorial

Improving Pagination step

tmnewt opened this issue · 2 comments

tmnewt commented

I think the Pagination step could benefit from an update.

When expanding the base template users are told to add a pagination block under the content block. Simple enough, right? Well, new users are still becoming acquainted with content blocks , and up until now in the tutorial their primary interaction with content blocks has been within templates extending the base template. I'd go so far as to wager that most failed to notice the content block tags when the base_generic.html file was introduced. Perceptive users might notice it during updating the sidebar-nav task but that was a while ago relative to this pagination task. To drive home my concern the tutorial instructions add to confusion with the line

...copy in the following pagination block below our content block (highlighted below in bold).

However, no highlighting is found.

My suggested fix would be to restate the entire base_generic.html file. Yes, it is a bit repetitive but given the length and depth of MDN's Django tutorial compared to its peers a little extra length can't hurt. Below should be all the html for the base up to this point in the tutorial.

<!DOCTYPE html>
<html lang="en">
<head>
  {% block title %}<title>Local Library</title>{% endblock %}
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bootstrap@4.5.3/dist/css/bootstrap.min.css" integrity="sha384-TX8t27EcRE3e/ihU7zmQxVncDAy5uIKz4rEkgIXeMed4M0jlfIDPvg6uqKI2xXr2" crossorigin="anonymous">
  
  <!-- Add additional CSS in static file -->
  {% load static %}
  <link rel="stylesheet" href="{% static 'css/styles.css' %}">
</head>
<body>
  <div class="container-fluid">
    <div class="row">
      <div class="col-sm-2">
      {% block sidebar %}
        <ul class="sidebar-nav">
          <li><a href="{% url 'index' %}">Home</a></li>
          <li><a href="{% url 'books' %}">All books</a></li>
          <li><a href="{% url 'authors'%}">All authors</a></li>
        </ul>
     {% endblock %}
      </div>
      <div class="col-sm-10 ">{% block content %}{% endblock %}

        <!-- pagination starts here-->
        {% block pagination %}
          {% if is_paginated %}
          <div class="pagination">
            <span class="page-links">
              {% if page_obj.has_previous %}
              <a href="{{ request.path }}?page={{ page_obj.previous_page_number }}">previous</a>
              {% endif %}
              <span class="page-current">
                Page {{ page_obj.number }} of {{ page_obj.paginator.num_pages }}.
              </span>
              {% if page_obj.has_next %}
              <a href="{{ request.path }}?page={{ page_obj.next_page_number }}">next</a>
              {% endif %}
            </span>
          </div>
          {% endif %}
        {% endblock %}
        <!-- pagination starts here-->

      </div>
    </div>
  </div>
</body>
</html>

Aside from my formatting the only things I've done here is explicitly point out the pagination block within the base template using comments. This will accomplish 2 things:

  1. Remind users that all templates can be edited to produce broad and immediate changes.
  2. Present users with an immediate example.
hamishwillee commented

@tmnewt Thank you for your suggestion. The highlighting is actually present in the source but not rendered - I'm in discussion on that here.

I have fixed this in the linked item by removing the bold highlighting and more clearly stating/showing where in the base template the update should be made. I fully appreciate that adding the full base template as you have shown would make it even more obvious. However I have not done this for two reasons:

  1. Highlighting doesn't work at the moment, so we'd have the same problem as you spotted for the existing text.
  2. It is better for maintenance not to show too much of the base template here - ie as things changes, I then don't have to updated this part too.

Hope that makes sense!

hamishwillee commented

PS II'm away on holiday until mid Jan. Sorry if I don't respond quickly.