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

Closed
chmac opened this issue Oct 11, 2022 · 5 comments
Closed

Feature Request: Add a confirmation on expense deletion #1077

chmac opened this issue Oct 11, 2022 · 5 comments

Comments

@chmac
Copy link

chmac commented Oct 11, 2022

I just deleted a duplicated expense in our budget, and if I'm not mistaken, it was instantly removed without any confirmation dialog. As deletion is a destructive operation, I'd be in favour of asking the user to confirm this action before carrying it out. This is the web UI which I'm referring to.

Even a simple globalThis.confirm() would be good enough from my perspective.

Side note, would you be open to a PR which adds this option? If yes, it would be great to get some pointers on where this code lives, if there's any translations to consider, and any suggestions on what message to use in the dialog. I'd go with "Are you sure?" as a first step otherwise.

@Glandos
Copy link
Member

Glandos commented Oct 12, 2022

I think a confirmation can be useful. Until we implemented an Undo feature, which I would much prefer.
Anyway, in the meantime, there is already that kind of confirmation in the project deletion workflow. The form is rendered here: https://github.com/spiral-project/ihatemoney/blob/master/ihatemoney/templates/edit_project.html#L36
And the little JS snippet to make a double click button is right above: https://github.com/spiral-project/ihatemoney/blob/master/ihatemoney/templates/edit_project.html#L5

You can try to make a confirmation based on this.

@chmac
Copy link
Author

chmac commented Nov 12, 2022

Thanks for the detailed pointers. I'm not sufficiently familiar with the codebase to attempt this kind of change I'm afraid. I was pretty confident I could find a spot to add the correct globalThis.prompt(), but that is a lot simpler.

I'm happy to leave this ticket open in case somebody else picks up on it and wants to submit a PR? Maybe adding the "good first issue" label would make sense?

@FlowingCloudRTL
Copy link
Contributor

Hello! is it ok for me to work on this issue?

@chmac
Copy link
Author

chmac commented Dec 1, 2022

@FlowingCloudRTL It seems like this is an issue which is open to any contributor, so I'd imagine that you're welcome to work on it. But just to be clear, I'm not a member of the project, so I don't know what the policies are for merging pull requests and so on.

@Glandos
Copy link
Member

Glandos commented Jan 8, 2023

Fixed by #1096

@Glandos Glandos closed this as completed Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants