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

[#8813] Add kiezradar delete modal #6002

Merged
merged 1 commit into from
Jan 29, 2025
Merged

Conversation

sevfurneaux
Copy link
Collaborator

@sevfurneaux sevfurneaux commented Jan 29, 2025

Describe your changes
This PR adds a delete modal for deleting kiezradars.

2025-01-29 13 59 02

Tasks

  • PR name contains story or task reference
  • Steps to recreate and test the changes
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@sevfurneaux sevfurneaux self-assigned this Jan 29, 2025
@sevfurneaux sevfurneaux force-pushed the sf-2025-kiezradar-add-delete-modal branch from 29ed879 to ff14a4f Compare January 29, 2025 13:39
@@ -138,3 +177,44 @@ function EditLink ({ to }) {
</Link>
)
}

function DeleteModal ({ onDelete, onClose }) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kept this component in the KiezradarList file as it wasn't used elsewhere. Can extract if that's a preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

see my message below

@sevfurneaux sevfurneaux force-pushed the sf-2025-kiezradar-add-delete-modal branch from ff14a4f to 52ea6f8 Compare January 29, 2025 13:56
@hom3mad3
Copy link
Contributor

hom3mad3 commented Jan 29, 2025

Hey @sevfurneaux, looks good, thanks! Just one thing: I recently refactored a4/Modal.jsx. We already have a delete modal, but it's currently only used in comments, so I didn't create a reusable one.

The changes aren’t merged yet: PR #1758.
Should we wait until Modal is merged and then move it over to a4? Do you want me to take this part over?

@sevfurneaux
Copy link
Collaborator Author

sevfurneaux commented Jan 29, 2025

Thanks @hom3mad3!

Is it ok if we merge this in, and then once the modals are completed/moved over to A4, we can retrospectively change this? The reason being, this implementation allows for #8751 [mB] Kiezradar frontend: managing Kieze on the "Ihre Kieze" ("Kieze verwalten") page (sev) to be completed, then Janek/Carolin can test the full kiezradar flow (creation, editing, deletion).

@hom3mad3
Copy link
Contributor

Thanks @hom3mad3!

Is it ok if we merge this in, and then once the modals are completed/moved over to A4, we can retrospectively change this? The reason being, this implementation allows for #8751 [mB] Kiezradar frontend: managing Kieze on the "Ihre Kieze" ("Kieze verwalten") page (sev) to be completed, then Janek/Carolin can test the full kiezradar flow (creation, editing, deletion).

sounds good, i will file an issue! thank you ✨

@hom3mad3 hom3mad3 merged commit cae3c89 into dev Jan 29, 2025
3 checks passed
@hom3mad3 hom3mad3 deleted the sf-2025-kiezradar-add-delete-modal branch January 29, 2025 16:16
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.

2 participants