Use hx-preserve with table checkboxes #148

cc-a · 2024-10-11T14:13:40Z

Description

This PR exempts the table checkboxes from being updated by HTMX requests. The main benefits:

removes issues with flickering checkboxes
allows removal of all server side handling of checkbox state

It does add the need to handle the header checkbox state to the client side but this is handled with a simple hyperscript snippet. For consistency I've also replaced the vanilla javascript with hyperscript to set row checkboxes when the header is clicked.

Type of change

Documentation (non-breaking change that adds or improves the documentation)
New feature (non-breaking change which adds functionality)
Optimization (non-breaking, back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)
Breaking change (whatever its nature)

Key checklist

All tests pass (eg. python -m pytest)
The documentation builds and looks OK (eg. python -m sphinx -b html docs docs/build)
Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

Code is commented, particularly in hard-to-understand areas
Tests added or an issue has been opened to tackle that in the future. (Indicate issue here: # (issue))

codecov-commenter · 2024-10-11T14:16:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.55%. Comparing base (9790d73) to head (939dae2).
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   79.54%   80.55%   +1.01%     
==========================================
  Files          26       26              
  Lines         308      324      +16     
==========================================
+ Hits          245      261      +16     
  Misses         63       63

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dalonsoa

It looks ok and it works, so approving. Having said that, I've lots of questions.

dalonsoa · 2024-10-12T09:11:09Z

process_manager/tables.py

 )
+
+ def render_select(self, value: str) -> str:


What's value here? Is the uuid of the column? Whatever it is, maybe add it to the docstring.

Also, I'm not sure when this method is called, or by whom.

Thanks good catch. I wish there was a way to get automatic checks for this but I've never been able to find one.

It's called during table rendering. It's detected by class magicness and called in place of the render method of the column.

mkdocs gives warnings about malformed docstrings (=errors if you pass --strict), but I don't know if it warns about missing args. Were we going to add API documentation at some point?

Planning to add it for next sprint.

dalonsoa · 2024-10-12T09:11:48Z

process_manager/tables.py

+ "th__input": {
+ "id": "header-checkbox",
+ "hx-preserve": "true",
+ "_": header_checkbox_hyperscript,


What does this underscore represent?

That is the html attribute that hyperscript uses to store its scripts on individual elements.

dalonsoa · 2024-10-12T09:13:00Z

process_manager/templates/process_manager/partials/process_table.html

@@ -1,4 +1,4 @@
 {% load render_table from django_tables2 %}
-<div hx-post="{% url 'process_manager:process_table' %}"
- hx-trigger="load delay:1s, click from:input"
+<div hx-get="{% url 'process_manager:process_table' %}"


Is it get now because we are no longer sending data (the selected rows) to the backend?

dalonsoa · 2024-10-12T09:17:29Z

process_manager/tables.py

+if <input.row-checkbox:not(:checked)/> is empty
+ set #header-checkbox.checked to true


In plain(er) English, this means that if the list of rows that are not checked is empty, then the header checkbox should be checked - as all the rows are checked - right?

cc-a · 2024-10-15T09:41:01Z

This has been up for a little while now so I'll merge by the end of the day unless anyone else has any feedback/questions.

alexdewar

LGTM insofar as I understand it 😉

alexdewar · 2024-10-15T09:54:08Z

process_manager/tables.py

 )
+
+ def render_select(self, value: str) -> str:


mkdocs gives warnings about malformed docstrings (=errors if you pass --strict), but I don't know if it warns about missing args. Were we going to add API documentation at some point?

tests/process_manager/views/test_partial_views.py

jamesturner246

Looks good. Just questions.

jamesturner246 · 2024-10-15T11:19:44Z

process_manager/tables.py

+ Args:
+ value: uuid from the table row data
+ """
+ return mark_safe(


So render_select is magically called when rendering self.select = tables.CheckBoxCol..., and creates a custom id for the row checkbox, so that hx-preserve can preserve each row cbox on refresh.

What is mark_safe doing here exactly?

https://docs.djangoproject.com/en/5.1/ref/utils/#django.utils.safestring.mark_safe It tells the Django templating system that the html generated is safe and isn't vurnerable to injection attacks so it can just be rendered directly. Otherwise the templating system by default will try to escape everything.

process_manager/tables.py

Co-authored-by: James Paul Turner <[email protected]> Co-authored-by: Christopher Cave-Ayland <[email protected]>

Co-authored-by: Alex Dewar <[email protected]>

Adds filtering

Use hx-preserve with table checkboxes

0f3ab2a

cc-a requested review from dalonsoa, jamesturner246, TinyMarsh, alexdewar, AdrianDAlessandro and Sahil590 October 11, 2024 14:13

dalonsoa approved these changes Oct 12, 2024

View reviewed changes

cc-a and others added 5 commits October 14, 2024 10:55

Expand docstring on ProcessTable.render_select

75392fc

🚧 Proof of concept filtering.

7aa02d7

✨ Add full search.

8ea49a3

✅ Add test for filtering table.

8717251

📝 Add explanatory comment.

fc29fa7

alexdewar approved these changes Oct 15, 2024

View reviewed changes

jamesturner246 approved these changes Oct 15, 2024

View reviewed changes

dalonsoa and others added 8 commits October 15, 2024 15:17

Apply suggestions from code review

4833da7

Co-authored-by: James Paul Turner <[email protected]> Co-authored-by: Christopher Cave-Ayland <[email protected]>

♻️ Remove search term and join-item.

6bcf273

♻️ Include more advance filtering.

25e5bbd

Update tests/process_manager/views/test_partial_views.py

ceef715

Co-authored-by: Alex Dewar <[email protected]>

Make css selectors in hyperscript snippets consistent

e53864c

♻️ Extract filter as a separate function.

987232d

Merge pull request #153 from ImperialCollegeLondon/filter

21f5e38

Adds filtering

Merge branch 'main' into preserve-checkboxes

939dae2

cc-a enabled auto-merge October 16, 2024 11:07

cc-a merged commit 90ab208 into main Oct 16, 2024
4 checks passed

cc-a deleted the preserve-checkboxes branch October 16, 2024 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use hx-preserve with table checkboxes #148

Use hx-preserve with table checkboxes #148

cc-a commented Oct 11, 2024

codecov-commenter commented Oct 11, 2024 •

edited

Loading

dalonsoa left a comment

dalonsoa Oct 12, 2024

cc-a Oct 14, 2024

alexdewar Oct 15, 2024

cc-a Oct 15, 2024

dalonsoa Oct 12, 2024

cc-a Oct 14, 2024

dalonsoa Oct 12, 2024

cc-a Oct 14, 2024

dalonsoa Oct 12, 2024

cc-a Oct 14, 2024

cc-a commented Oct 15, 2024

alexdewar left a comment

alexdewar Oct 15, 2024

jamesturner246 left a comment

jamesturner246 Oct 15, 2024

cc-a Oct 15, 2024

		if <input.row-checkbox:not(:checked)/> is empty
		set #header-checkbox.checked to true

Use hx-preserve with table checkboxes #148

Use hx-preserve with table checkboxes #148

Conversation

cc-a commented Oct 11, 2024

Description

Type of change

Key checklist

Further checks

codecov-commenter commented Oct 11, 2024 • edited Loading

Codecov Report

dalonsoa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cc-a commented Oct 15, 2024

alexdewar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesturner246 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 11, 2024 •

edited

Loading