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 django-tables2 and utils, example, docs for usage with HTMX #35470

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

biyeun
Copy link
Member

@biyeun biyeun commented Dec 3, 2024

Technical Summary

This PR introduces a new python dependency django-tables2, which we will use to paginate and sort tabular data. In combination with HTMX, this library allows us to quickly render partial template responses of a table's page based on a queryset and a requests GET parameters. In combination with HTMX, this introduces a powerful way to easily asynchronously paginate through tabular data.

To support this combination of libraries, this PR introduces:

  • custom django-tables2 templates for Bootstrap 5 tables and Bootstrap 5 tables with HTMX
  • A SelectablePaginatedTableView base class based on django-tables2 SingleTableView which handles the widget selecting page length and saving that selection to a cookie (and fetching the saved value later)—similar to how our Knockout pagination component handles selecting pagination length.
  • New scss styles for sortable headers in a django-tables2 table
  • A new custom hq-hx plugin called hq-hx-loading for triggering a loading overlay (in addition to the usual hx-indicator functionality...you can't easily do both at once with hx-indicator). We use this in our django-tables2 HTMX table.
  • A new hq-hx-refresh plugin for "chaining" an hqRefresh event to a target selector (the value of the hq-hx-refresh attribute) once the HTMX request on the originating element with that attribute completes. (hq-hx-refresh-after for htmx:afterRequest and hq-hx-refresh-swap for htmx:afterSwap).
  • A BaseHTmxTable class to make setting up the django-tables2 table definition object easier.
  • A working example added to the styleguide using all the features described above.
  • Documentation updates

Screenshot 2024-12-03 at 9 19 33 PM
Screenshot 2024-12-03 at 9 20 20 PM
Screenshot 2024-12-03 at 9 20 00 PM
Screenshot 2024-12-03 at 9 19 45 PM

Safety Assurance

Safety story

Safe change. Documentation updates and utilities. No changes to active pages.

Automated test coverage

No

QA Plan

Not needed

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@biyeun biyeun added product/all-users-all-environments Change impacts all users on all environments dependencies Pull requests that update a dependency file labels Dec 3, 2024
@biyeun biyeun requested review from mjriley, orangejenny, jingcheng16, nospame and a team December 3, 2024 21:19
corehq/apps/hqwebapp/templatetags/hq_tables_tags.py Outdated Show resolved Hide resolved
corehq/apps/hqwebapp/tables/pagination.py Outdated Show resolved Hide resolved
hx-get="{{ request.path_info }}"
hx-target="{% if table.css_id %}#{{ table.css_id }}{% else %}div.table-container{% endif %}"
hx-swap="outerHTML"
hq-hx-loading="{{ table.loading_indicator_id }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we might be a little past the initial introduction of hq-hx-..., but I find it awkward to pronounce and alphabet-soupy. Can we use a different prefix like hqx- or maybe even hq- (without the hx- part)?

Similarly for things named hq_hx_..., the hq_ prefix seems unnecessary since all of our custom utilities and extensions to HTMX live in the corehq package/namespace, so adding an extra hq seems like needless duplication in addition to awkward and alphabet-soupy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the shortened name, but I feel it abstracts the hx- (for HTMX) connection a little too much. It's clear from hq-hx- that this is a custom HQ HTMX attribute. And since Alpine attributes begin with x-, hqx- seems more ambiguous as it could reference alpine, not HTMX. I would prefer to keep the naming scheme as is. Regardless, I feel such a change would be out of scope for this PR given that there are other extensions existing that already a part of HQ.


This is useful for adding loading indicators to elements outside the parent heirarchy available
through using `hx-indicator` alone. Right now, this is used to add an `is-loading` style to a django tables
table, which overlays a loading indicator across the entire table (seen in hqwebapp/tables/bootstrap5_htmx.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

ensures nothing in the table can be interacted with until the HTMX request is complete

Is it possible that an error or similar outcome to the HTMX request could cause this loading indicator to not be removed? That seems like it could be disruptive to page interactions.

For usability it would be preferable to limit the scope of interactivity barriers to the smallest area possible. Any chance the scope could be reduced to a single table cell or row?

Caveat: I have not seen this in type of disable whole table workflow in real life before, so maybe it's not as bad as I am imagining?

Copy link
Member Author

@biyeun biyeun Dec 17, 2024

Choose a reason for hiding this comment

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

it's important for paginating the table that this workflow is in place, as clicking on an edit button, select row, etc during pagination would result in issues. Similarly during the whole table refresh process when filters are applied. I think if you see the data cleaning example on staging https://staging.commcarehq.org/prototype/dc/ (you need the FF enabled), then that will give you the interactive example.

TL;DR Cell-specific actions are already limited. The whole-table barrier is intentional and necessary.

I believe an error would still fire that event. Regardless, if there is an error on pagination, I think we have bigger issues...I would want a disruptive behavior. In any case, a page refresh is always possible.

class MyNewTable(BaseHtmxTable):

class Meta(BaseHtmxTable.Meta):
pass # or you can override or add table attributes here -- see Django Tables docs
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
pass # or you can override or add table attributes here -- see Django Tables docs
... # override or add table attributes here -- see Django Tables docs

Copy link
Member Author

Choose a reason for hiding this comment

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

pass is actually intentional. It's always required, unless overrides or additions are happening.

Comment on lines +22 to +45
name = columns.Column(
verbose_name=gettext_lazy("Name"),
)
color = columns.Column(
verbose_name=gettext_lazy("Color"),
)
big_cat = columns.Column(
verbose_name=gettext_lazy("Big Cats"),
)
dob = columns.Column(
verbose_name=gettext_lazy("Date of Birth"),
)
app = columns.Column(
verbose_name=gettext_lazy("Application"),
)
date_opened = columns.Column(
verbose_name=gettext_lazy("Opened On"),
)
owner = columns.Column(
verbose_name=gettext_lazy("Owner"),
)
status = columns.Column(
verbose_name=gettext_lazy("Status"),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
name = columns.Column(
verbose_name=gettext_lazy("Name"),
)
color = columns.Column(
verbose_name=gettext_lazy("Color"),
)
big_cat = columns.Column(
verbose_name=gettext_lazy("Big Cats"),
)
dob = columns.Column(
verbose_name=gettext_lazy("Date of Birth"),
)
app = columns.Column(
verbose_name=gettext_lazy("Application"),
)
date_opened = columns.Column(
verbose_name=gettext_lazy("Opened On"),
)
owner = columns.Column(
verbose_name=gettext_lazy("Owner"),
)
status = columns.Column(
verbose_name=gettext_lazy("Status"),
)
name = columns.Column(verbose_name=gettext_lazy("Name"))
color = columns.Column(verbose_name=gettext_lazy("Color"))
big_cat = columns.Column(verbose_name=gettext_lazy("Big Cats"))
dob = columns.Column(verbose_name=gettext_lazy("Date of Birth"))
app = columns.Column(verbose_name=gettext_lazy("Application"))
date_opened = columns.Column(verbose_name=gettext_lazy("Opened On"))
owner = columns.Column(verbose_name=gettext_lazy("Owner"))
status = columns.Column(verbose_name=gettext_lazy("Status"))

Copy link
Member Author

Choose a reason for hiding this comment

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

eh...I find the other format more readable tbh

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can agree to disagree. I find the compact style MUCH more readable.

Comment on lines +19 to +20
class Meta(BaseHtmxTable.Meta):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary since it's defining no overrides or new properties, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

no it is absolutely necessary. it is inheriting from BaseHtmxTable.Meta.

class Foo(BaseHtmxTable):
    ...

this by itself is not enough if BaseHtmxTable has class Meta inside of it. Believe me, I tried. This is the way.

Copy link
Contributor

@millerdev millerdev Dec 18, 2024

Choose a reason for hiding this comment

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

That's unfortunate and not how Django model classes work. It should be possible to add an empty/default Meta class to any BaseHtmxTable subclass that does not define it. Might be able to do it with a "metaclass" (not to be confused with the Meta class).

Copy link
Member Author

Choose a reason for hiding this comment

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

That creates some complications as there is already a metaclass for a django-tables2 table that references the attributes defined in Meta. I spent some time trying to figure out a workaround, but this was the simplest way that didn't add additional abstractions to the expected behavior from django-tables2 At least the same attributes don't have to be re-writted each time the same kind of htmx table is used

table_class = ExampleFakeDataTable

def get_queryset(self):
return generate_example_pagination_data(100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this expected to be a query that returns a single page of data? If yes, how does it know what page to fetch?

Edit: on further investigation I think this is expected to return a queryset that would fetch all possible rows for the table, which is later sliced into pages. Note that django_tables2.paginators.LazyPaginator as well as django.core.paginator.Paginator use SQL LIMIT and OFFSET, which is not suitable for deep pagination on large data sets. Have you investigated how easy it is to do efficient pagination on very large data sets (using a WHERE clause rather than OFFSET to locate the beginning of the page)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not yet. I'm investigating that after I return from PTO in January. This is intentionally simple as it's just an example. It will likely involve a custom paginator class, I'm sure of it...especially with elasticsearch data.

"""
This view returns a partial template of a table, along with its
page controls and page size selection. Its parent classes handle
pagination of a given queryset based on GET parameters in the request.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the code for pagination of a given queryset based on GET parameters in the request? I don't see where GET parameters can be used to filter the queryset. See also comment on get_queryset.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's part of the parent classes this class inherits from that's built into the django-tables2 library.

The pages themselves are handled in the template.

see

{% block select-per-page-attr %}
name="{{ table.per_page_field }}"
hx-get="{{ request.path_info }}"
hx-target="{% if table.container_id %}#{{ table.container_id }}{% else %}div.table-container{% endif %}"
hx-swap="outerHTML"
hq-hx-loading="{{ table.loading_indicator_id }}"
{% endblock %}
{% block prev-page-link-attr %}
hx-get="{{ request.path_info }}{% querystring table.prefixed_page_field=table.page.previous_page_number %}"
hx-replace-url="{% querystring table.prefixed_page_field=table.page.previous_page_number %}"
hx-trigger="click"
hx-target="{% if table.container_id %}#{{ table.container_id }}{% else %}div.table-container{% endif %}"
hx-swap="outerHTML"
hq-hx-loading="{{ table.loading_indicator_id }}"
{% endblock %}
{% block next-page-link-attr %}
hx-get="{{ request.path_info }}{% querystring table.prefixed_page_field=table.page.next_page_number %}"
hx-replace-url="{% querystring table.prefixed_page_field=table.page.next_page_number %}"
hx-trigger="click"
hx-target="{% if table.container_id %}#{{ table.container_id }}{% else %}div.table-container{% endif %}"
hx-swap="outerHTML"
hq-hx-loading="{{ table.loading_indicator_id }}"
{% endblock %}
{% block pagination.range %}
{% for p in table.page|table_page_range:table.paginator %}
<li class="page-item{% if table.page.number == p %} active{% endif %}">
<a
class="page-link"
{% if p != '...' %}
hx-get="{{ request.path_info }}{% querystring table.prefixed_page_field=p %}"
hx-replace-url="{% querystring table.prefixed_page_field=p %}"
hx-trigger="click"
hx-target="{% if table.container_id %}#{{ table.container_id }}{% else %}div.table-container{% endif %}"
hx-swap="outerHTML"
hq-hx-loading="{{ table.loading_indicator_id }}"
{% endif %}
>
{{ p }}
</a>
</li>
{% endfor %}
{% endblock pagination.range %}

and

{% block pagination %}
{% if table.page and table.paginator.num_pages > 1 %}
<nav aria-label="Table navigation">
<ul class="pagination">
{% block pagination.previous %}
<li class="previous page-item{% if not table.page.has_previous %} disabled{% endif %}">
<a
class="page-link"
{% if table.page.has_previous %}
{% block prev-page-link-attr %}
href="{% querystring table.prefixed_page_field=table.page.previous_page_number %}"
{% endblock %}
{% endif %}
>
{% trans 'Previous' %}
</a>
</li>
{% endblock pagination.previous %}
{% if table.page.has_previous or table.page.has_next %}
{% block pagination.range %}
{% for p in table.page|table_page_range:table.paginator %}
<li class="page-item{% if table.page.number == p %} active{% endif %}">
<a
class="page-link"
{% if p != '...' %}
href="{% querystring table.prefixed_page_field=p %}"
{% endif %}
>
{{ p }}
</a>
</li>
{% endfor %}
{% endblock pagination.range %}
{% endif %}
{% block pagination.next %}
<li class="next page-item{% if not table.page.has_next %} disabled{% endif %}">
<a
class="page-link"
{% if table.page.has_next %}
{% block next-page-link-attr %}
href="{% querystring table.prefixed_page_field=table.page.next_page_number %}"
{% endblock %}
{% endif %}
>
{% trans 'Next' %}
</a>
</li>
{% endblock pagination.next %}
</ul>
</nav>
{% endif %}
{% endblock pagination %}

for usage within our templates.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original question was poorly worded. I was not asking how the templates use HTMX to add GET parameters to the request. I was asking where is the code that retrieves GET parameters from the request to construct/filter the database query? Is that code easy to extend or is it deep in the framework somewhere where where it is difficult to customize?

Based on this comment it sounds like you will be looking into this more later. It feels like an important design detail in this PR, so it doesn't feel great to sign off on this without understanding more about how that will work.

Copy link
Member Author

Choose a reason for hiding this comment

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

class SelectablePaginatedTableView(SelectablePaginatedTableMixin, ListView):
"""
Based on SingleTableView, which inherits from `SingleTableMixin`, `ListView`
we instead extend the `SingleTableMixin` with `SavedPaginatedTableMixin`.
"""

SelectablePaginatedTableView subclasses ListView, which is a built-in django view that handles the pagination based on a given paginator and data source. the table mixin provides the tabular data source and structure for the template tags

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, digging into the code, the pagination happens from django's Paginator. django-tables2 formats the data for display, but ultimately it's django's core classes that are doing the work, and they seem extendable.

@biyeun
Copy link
Member Author

biyeun commented Dec 17, 2024

@millerdev ready for a re-review! made changes and left responses. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file product/all-users-all-environments Change impacts all users on all environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants