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

Only update affected media list when updating destination in HTMX frontend #1121

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Jan 10, 2025

Fixes #1136

Makes it so only the related list for the destination you try to delete is updated with HTMX.

@stveit stveit self-assigned this Jan 10, 2025
@stveit stveit force-pushed the htmx-destination-delete-only-affected-row branch from a4325bf to fdfda05 Compare January 10, 2025 15:11
@stveit stveit force-pushed the htmx-destination-update-only-affected-row branch from f66e72e to 5d0426b Compare January 10, 2025 15:12
Copy link

github-actions bot commented Jan 10, 2025

Test results

   10 files  1 060 suites   38m 23s ⏱️
  536 tests   535 ✅  1 💤 0 ❌
5 360 runs  5 350 ✅ 10 💤 0 ❌

Results for commit 34dd036.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 7.69231% with 12 lines in your changes missing coverage. Please review.

Project coverage is 79.62%. Comparing base (8b98bad) to head (34dd036).

Files with missing lines Patch % Lines
src/argus/htmx/destination/views.py 7.69% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
- Coverage   79.67%   79.62%   -0.05%     
==========================================
  Files         141      141              
  Lines        5288     5292       +4     
==========================================
+ Hits         4213     4214       +1     
- Misses       1075     1078       +3     

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

@stveit stveit force-pushed the htmx-destination-update-only-affected-row branch from 5d0426b to fe961ed Compare January 10, 2025 18:23
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Should also have a news fragment (could also be one for #1120 and this PR together), otherwise good

Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Nice! See a RFC below

@@ -1,6 +1,7 @@
<form hx-post="{% url 'htmx:htmx-update' form.instance.id %}"
hx-trigger="submit"
hx-target="#form-list"
hx-target="closest #update-delete-forms"
Copy link
Contributor

Choose a reason for hiding this comment

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

Having same id on several HTML tags is a bad practice. We could either make an id of each form unique with something like update-delete-form-{{ media.name }}-{{ forloop.counter }}, or put a class f.e. update-delete-form on each form and target it as closest .update-delete-form. The latter is easiest

Copy link
Contributor

Choose a reason for hiding this comment

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

If prefix is set when instantiating the form, it is available via {{form.prefix}}. This can then be used to give the form itself an id. However, this also adds the prefix to every field-id (must be unique per page) and field-name, so that every form dealing with the same data must have the same prefix set. See #1118 for prefix-wrangling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was changed to not rely on ID anymore, now it uses "closest detail", finding the closest collapse element

@hmpf
Copy link
Contributor

hmpf commented Jan 14, 2025

There will never be many destinations, a refresh of the entire page is also fine and will greatly simplify matters I suspect.

@stveit stveit force-pushed the htmx-destination-delete-only-affected-row branch 4 times, most recently from 1cccf5e to 4e46505 Compare January 16, 2025 08:21
Base automatically changed from htmx-destination-delete-only-affected-row to master January 16, 2025 12:10
@stveit stveit force-pushed the htmx-destination-update-only-affected-row branch 5 times, most recently from 31dcbda to 02d7157 Compare January 16, 2025 12:49
@stveit stveit changed the title Only update affected row when updating destination in HTMX frontend Only update affected media list when updating destination in HTMX frontend Jan 16, 2025
@stveit
Copy link
Contributor Author

stveit commented Jan 16, 2025

Changed PR to focus on updating the entire list and not just the row

@stveit stveit force-pushed the htmx-destination-update-only-affected-row branch from 02d7157 to 5eded9e Compare January 16, 2025 12:52
@stveit stveit force-pushed the htmx-destination-update-only-affected-row branch from 5eded9e to 34dd036 Compare January 17, 2025 07:39
@johannaengland johannaengland self-requested a review January 20, 2025 10:43
Copy link
Contributor

@podliashanyk podliashanyk left a comment

Choose a reason for hiding this comment

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

Works nicely!

The only UX issue I see is that the error message on destination delete is shown not by the destination in question, but at the top of the media list which is confusing. But this issue was likely introduced in #1120 and not in this PR.

@stveit stveit merged commit 3d8d6ca into master Jan 20, 2025
17 checks passed
@stveit stveit deleted the htmx-destination-update-only-affected-row branch January 21, 2025 08:10
@stveit
Copy link
Contributor Author

stveit commented Jan 21, 2025

Works nicely!

The only UX issue I see is that the error message on destination delete is shown not by the destination in question, but at the top of the media list which is confusing. But this issue was likely introduced in #1120 and not in this PR.

#1143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update only the related media list when updating a destination
5 participants