Refactored convert pagination to django-components structure.

[Antoliny0919]

I refactoring the existing pagination using django-components.

[Antoliny0919]

@JuroOravec
It’s a bit nerve wracking to get a review from a maintainer, but thank you so much ☺️

[JuroOravec]

You can add input validation by defining a Kwargs class. When you do so, then the kwargs argument to get_template_data() will be an instance of Kwargs (instead of plain dict).

This will have the effect of input validation, because if we render the component without pagination_obj then the instantiation of NamedTuple will raise an error:

@register("pagination")
class PaginationComponent(Component):
    template_file = "pagination.html"

    # This will raise if `pagination_obj` is missing
    class Kwargs(NamedTuple):
        pagination_obj: Pagination

    def get_template_data(self, args, kwargs: Kwargs, slots, context):
        pagination = kwargs.pagination_obj
        # Rest of the code...

The limitation of using NamedTuple for Kwargs is that it will only check the pagination_obj input is given, but it won't check that pagination_obj is of correct type.

You could potentially also get rid of the input validation on lines 34-35. For that you'd have to add Pydantic as a dependency. When you use Pydantic's BaseModel for defining Kwargs, then Pydantic will check both the shape AND the types of individual fields.

So overall to not have to do component input validation by hand, the code could look like this:

from django_components import Component, register
from django.utils.html import format_html, format_html_join
from pydantic import BaseModel  # NEW

from base.pagination import  PAGE_VAR, Pagination
from base.templatetags.base_templatetags import querystring


@register("pagination")
class PaginationComponent(Component):
    template_file = "pagination.html"

    # This will raise if `pagination_obj` is missing or of wrong type
    class Kwargs(BaseModel):
        pagination_obj: Pagination

    def get_template_data(self, args, kwargs: Kwargs, slots, context):
        pagination = kwargs.pagination_obj
        page_elements = format_html_join(
        # Rest of the code...

[Antoliny0919]

Using Kwargs with Pydantic definitely makes input validation much cleaner.
Type hints haven’t been applied in djangosnippets yet, but I think it’s worth considering adding them.
Thanks again!

[JuroOravec]

Following is just my personal preference - when writing components I try to keep HTML and CSS inside the templates, so different languages mix as little as possible (if possible).

Here you are rendering a list of page numbers and you are rendering then as HTML, but still from within Python. I'd move that in place of the {{ page_elements }} on line 10 in pagination.html.

One way how that could be done could be to define data objects and then simply render each item based on the data object's state:

class PaginationItem(NamedTuple):
    kind: Literal["current", "ellipsis", "number"]
    text: str | None = None
    attrs: dict | None = None

@register("pagination")
class PaginationComponent(Component):
    template_file = "pagination.html"

    def pagination_number(self, pagination: Pagination, num: int | str) -> PaginationItem:
        if num == pagination.paginator.ELLIPSIS:
            return PaginationItem(kind="ellipsis", text=pagination.paginator.ELLIPSIS)
        elif num == pagination.page_num:
            return PaginationItem(kind="current", text=num)
        else:
            link = querystring(None, {**pagination.params, PAGE_VAR: num})
            return PaginationItem(
                kind="number",
                text=num,
                attrs={"href": link},
            )

    def get_template_data(self, args, kwargs, slots, context):
        page_elements =  self.pagination_number(pagination, page_num)} for page_num in pagination.page_range

        ...

        page_link_css = "py-[5px] px-[10px] border-1 border-transparent rounded-lg no-underline hover:border-base-orange-400"

        return {
            "pagination": pagination,
            "previous_page_link": previous_page_link,
            "next_page_link": next_page_link,
            "page_elements": page_elements,
            "page_link_css": page_link_css,
        }

And then we could move all the CSS and HTML to the template. We'd replace line 10 of the HTML template with following:

{% for page_item in page_elements %}
  <li class="inline-block">
    {% if page_item.kind == "ellipsis" %}
        {{ page_item.text }}
    {% if page_item.kind == "current" %}
        <a
            href=""
            class="{{ page_link_css }} bg-base-orange-400 text-base-white-400"
            aria-current="page"
        >
            {{ page_item.text }}
        </a>
    {% else %}
        <a
            {% html_attrs
                page_item.attrs
                class=page_link_css
            %}
        >
            {{ page_item.text }}
        </a>
    {% endif %}
  </li>
{% endfor %}

[Antoliny0919]

I agree.
The approach you suggested really feels like it maximizes the strengths of using components.
pagination_number method determines the attributes of each pagination item,
and leaving the actual rendering to the HTML makes the structure much cleaner and better overall 🙌

[JuroOravec]

Since there is no content between the {% component %} and {% endcomponent %} tags, we could shorten it with self-closing /:

{% component "pagination" pagination_obj=pagination / %}

Same applies also to other files where this component was used.

[JuroOravec]

@Antoliny0919 The use of django-components looks good to me, left a few suggestions :)

Can't comment on the PR from the perspective of the djangosnippets project, someone else will have to take a look.

[Antoliny0919]

Thank you for taking the time to review this despite your busy schedule @JuroOravec ⭐️
Your suggestions have been a great help in guiding the direction I should take for future implementations using django-components.

[Antoliny0919]

I think the other PRs should be merged first, and this PR should be merged last 🙌

[chriswedgwood]

Great work!

[chriswedgwood]

we are getting this in heroku

Its from one of the prs I just merged

[Antoliny0919]

we are getting this in heroku

Its from one of the prs I just merged

Is it possible to test the Heroku environment locally?

That said, even though I don't know the exact cause, I think it's safe to remove this part now, since the base TEMPLATES configuration has been updated.

# Use the cached template loader.
del TEMPLATES[0]["APP_DIRS"]
TEMPLATES[0]["OPTIONS"]["loaders"] = (
    (
        "django.template.loaders.cached.Loader",
        (
            "django.template.loaders.filesystem.Loader",
            "django.template.loaders.app_directories.Loader",
        ),
    ),
)

[Antoliny0919]

I suspect it might be because APP_DIRS was already removed in the base TEMPLATES configuration, and then it's being removed again in the production settings. 🧐

[Antoliny0919]

@chriswedgwood
Could you please check it one more time? If the issue still occurs, please let me know.