Skip to content

Conversation

@hmpf
Copy link
Contributor

@hmpf hmpf commented Apr 4, 2025

Scope and purpose

Fixes #1357, depends on #1358

Contributor Checklist

Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.

  • Added a changelog fragment for towncrier
  • Linted/formatted the code with ruff and djLint, easiest by using pre-commit
  • The first line of the commit message continues the sentence "If applied, this commit will ...", starts with a capital letter, does not end with punctuation and is 50 characters or less long. See our how-to
  • If applicable: Created new issues if this PR does not fix the issue completely/there is further work to be done
  • If this results in changes in the UI: Added screenshots of the before and after

@hmpf hmpf changed the title Make destination delete modal Make destination delete button use a modal Apr 4, 2025
@hmpf hmpf requested review from podliashanyk and stveit April 4, 2025 12:41
@hmpf hmpf added the frontend Affects frontend label Apr 4, 2025
@hmpf hmpf self-assigned this Apr 4, 2025
@hmpf hmpf moved this from 📋 Backlog to ❓ Ready for review in Argus development, public Apr 4, 2025
@hmpf hmpf marked this pull request as ready for review April 4, 2025 12:44
@hmpf hmpf requested a review from johannaengland April 4, 2025 12:44
@github-actions
Copy link

github-actions bot commented Apr 4, 2025

Test results

   11 files  1 320 suites   49m 59s ⏱️
  583 tests   582 ✅  1 💤 0 ❌
6 413 runs  6 402 ✅ 11 💤 0 ❌

Results for commit 29697c3.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2025

Codecov Report

Attention: Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.

Project coverage is 78.58%. Comparing base (b388959) to head (29697c3).

Files with missing lines Patch % Lines
src/argus/htmx/destination/views.py 41.17% 10 Missing ⚠️
src/argus/notificationprofile/models.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1359      +/-   ##
==========================================
- Coverage   78.74%   78.58%   -0.16%     
==========================================
  Files         142      142              
  Lines        5800     5814      +14     
==========================================
+ Hits         4567     4569       +2     
- Misses       1233     1245      +12     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@stveit stveit left a comment

Choose a reason for hiding this comment

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

Seems to be working fine, smol thing that sonarcloud picked up on

@github-project-automation github-project-automation bot moved this from ❓ Ready for review to ♻ Changes requested in Argus development, public Apr 7, 2025
@hmpf hmpf requested a review from stveit April 7, 2025 08:18
@github-project-automation github-project-automation bot moved this from ♻ Changes requested to 👍 Reviewer approved in Argus development, public Apr 7, 2025
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.

When I try deleting a destination that is connected to a profile I get the following response:

image

So somehow it leads to the rest of the page disappearing

form = DestinationUpdateForm(instance=obj, prefix=f"destination_{obj.pk}")
form.modal = DeleteModal(
header="Delete destination",
explanation=f'Delete destination "{obj}"',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
explanation=f'Delete destination "{obj}"',
explanation=f'Delete destination "{obj}"?',

@github-project-automation github-project-automation bot moved this from 👍 Reviewer approved to ♻ Changes requested in Argus development, public Apr 7, 2025
<div class="flex w-full h-fit items-center gap-2 justify-center">
{% include "htmx/destination/_edit_form.html" %}
{% include "htmx/destination/_delete_form.html" %}
{% include form.modal.template_name with modal=form.modal %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Then htmx/destination/_delete_form.html can be deleted

Base automatically changed from make-delete-modal to master April 7, 2025 11:50
@hmpf hmpf force-pushed the make-destination-delete-modal branch from c274506 to 29697c3 Compare April 7, 2025 11:55
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 7, 2025

@hmpf hmpf added the paused Assignee is busy with things of higher priority label Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

consistency frontend Affects frontend paused Assignee is busy with things of higher priority

Projects

Status: Changes requested

Development

Successfully merging this pull request may close these issues.

Use new style modal for all delete buttons

5 participants