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 Htmx destinations page #970

Merged
merged 11 commits into from
Dec 9, 2024
Merged

Add Htmx destinations page #970

merged 11 commits into from
Dec 9, 2024

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Nov 27, 2024

Closes #1001

Replaces Uninett/argus-htmx-frontend#205

Things that need to be done:

Sonarcloud complains about A form label must be associated with a control. , but the form field is inside the label so explicitly stating the control element shouldnt be necessary.

Copy link

github-actions bot commented Nov 27, 2024

Test results

   10 files  1 040 suites   35m 43s ⏱️
  529 tests   528 ✅  1 💤 0 ❌
5 290 runs  5 280 ✅ 10 💤 0 ❌

Results for commit 993ba78.

♻️ This comment has been updated with latest results.

@hmpf hmpf changed the title Htmx destinations page Add Htmx destinations page Nov 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 35.21127% with 92 lines in your changes missing coverage. Please review.

Project coverage is 81.19%. Comparing base (74011a1) to head (993ba78).

Files with missing lines Patch % Lines
src/argus/htmx/destination/forms.py 36.11% 46 Missing ⚠️
src/argus/htmx/destination/views.py 29.23% 46 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #970      +/-   ##
==========================================
- Coverage   82.50%   81.19%   -1.31%     
==========================================
  Files         138      140       +2     
  Lines        4944     5074     +130     
==========================================
+ Hits         4079     4120      +41     
- Misses        865      954      +89     

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

@stveit stveit force-pushed the htmx-destinations branch 5 times, most recently from 301a331 to c1e3d53 Compare December 4, 2024 18:21
@stveit stveit marked this pull request as ready for review December 4, 2024 18:21
@stveit stveit force-pushed the htmx-destinations branch 2 times, most recently from b97d9e0 to 7f55846 Compare December 5, 2024 16:49
@hmpf
Copy link
Contributor

hmpf commented Dec 9, 2024

Sonarcloud complains about A form label must be associated with a control. , but the form field is inside the label so explicitly stating the control element shouldnt be necessary.

It's that stupid, is it? sigh

@hmpf hmpf requested review from hmpf, lunkwill42, podliashanyk and johannaengland and removed request for hmpf December 9, 2024 09:00
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

Manually tested, seems to work just fine. @podliashanyk?

Pretty much copied from the destinations API, needs some changes
so it wont just crash to 500 when it raises a ValidationError

This commit doesnt make it show in the frontend, the button for
deleting a destination will be grouped with the forms for
updating destinations
@stveit stveit force-pushed the htmx-destinations branch from 7f55846 to 993ba78 Compare December 9, 2024 11:21
Copy link

sonarqubecloud bot commented Dec 9, 2024

@podliashanyk
Copy link
Contributor

Sonarcloud complains about A form label must be associated with a control. , but the form field is inside the label so explicitly stating the control element shouldnt be necessary.

It's that stupid, is it? sigh

Yup. All input fields on destinations page pass the test "click on a label and see the input field being activated". So all labels ARE correctly programatically connected to corresponding input fields

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.

Great job! 👏 The page works and looks very well. See some non-critical suggestions/polishes/bugs below (sorted by priority). All of them belong to separate PRs.

  1. Bug: when creating an email and an sms destination with the same name I get error "Name must be unique per media".
  2. Error text should be red.
  3. Applies to collapse elements, as well as the create form: let's add vertical gaps between them, also let's add padding/spacing on each side of them, and let's center their contents.
  4. See HTMX suggestions below. All of them are polishes to make swaps more granular.

Comment on lines +7 to +10
<div class="flex w-full h-fit items-center">
{% include "htmx/destination/_edit_form.html" %}
{% include "htmx/destination/_delete_form.html" %}
</div>
Copy link
Contributor

@podliashanyk podliashanyk Dec 9, 2024

Choose a reason for hiding this comment

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

HTMX suggestion: If we put id on the parent div (the one with class .collapse-content) and put a highlighted div (lines 7-10) into a separate file, then we can return that file in create and update endpoints. This logic combined with using hx-swap='beforeend' would make more granular swaps on create and update actions.

Comment on lines +1 to +3
<form hx-post="{% url 'htmx:htmx-delete' form.instance.id %}"
hx-trigger="submit"
hx-target="#form-list">
Copy link
Contributor

@podliashanyk podliashanyk Dec 9, 2024

Choose a reason for hiding this comment

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

HTMX suggestion: If we put id on each form-div, we could use it as a value in hx-target here. This combined with hx-swap='delete' would lead to a more granular swap on delete action.

@hmpf hmpf merged commit 0e44800 into master Dec 9, 2024
17 checks passed
@hmpf hmpf deleted the htmx-destinations branch December 9, 2024 12:46
@stveit stveit mentioned this pull request Dec 10, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Make destinations-page
4 participants