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

Feature Request: Add a confirmation on expense deletion #1077 #1096

Conversation

cbrosnan
Copy link
Contributor

@cbrosnan cbrosnan commented Dec 6, 2022

Completed feature request #1077 to use the double-click confirmation button that is also found in edit_project.html. This will prevent the accidental deletion of expenses (a destructive operation) with no warning.

@almet
Copy link
Member

almet commented Dec 11, 2022

This looks good to me. I feel we could refactor some code with the exact same thing happening with user deletion (just a few lines up in the JS). What do you think?

@almet
Copy link
Member

almet commented Dec 11, 2022

Also, see #1099

@cbrosnan
Copy link
Contributor Author

I followed the JS convention that @Glandos pointed out in the original feature request (#1077). It is the same as the confirmation that is found in the project deletion workflow. I personally thought that the design looked better and drew better attention to the expense deletion than the user deletion confirmation. What are your thoughts?

@almet
Copy link
Member

almet commented Dec 11, 2022

I thought originally that we could do some refactoring, but you're right : let's merge this :-) Thanks

@almet almet merged commit 4bef5ad into spiral-project:master Dec 11, 2022
Glandos added a commit to Glandos/ihatemoney that referenced this pull request Jan 28, 2023
Now the "danger" strings appears nearly everywhere, but the test looks for a flash message, thus "alert-danger"
Glandos added a commit that referenced this pull request Jan 28, 2023
Now the "danger" strings appears nearly everywhere, but the test looks for a flash message, thus "alert-danger"
TomRoussel pushed a commit to TomRoussel/ihatemoney that referenced this pull request Mar 2, 2024
TomRoussel pushed a commit to TomRoussel/ihatemoney that referenced this pull request Mar 2, 2024
Now the "danger" strings appears nearly everywhere, but the test looks for a flash message, thus "alert-danger"
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.

None yet

2 participants