Skip to content

confirmation dialog on upload/download/remove files #85

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

Closed

Conversation

ubidefeo
Copy link
Collaborator

confirmation is delegated to a single function returning true/false based on message.
this solution is a temporary move towards having UI based confirmation panels

@ubidefeo ubidefeo requested a review from murilopolese January 12, 2024 16:10
Copy link
Contributor

@murilopolese murilopolese left a comment

Choose a reason for hiding this comment

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

I would totally skip the confirmationDialog function. In case we really need it, it would be better with the other helper function at the end of the file.

Comment on lines 531 to 535
function confirmDialog(message, cancelText, confirmText) {
confirmation = confirm(message, cancelText, confirmText);
return confirmation;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This helper is being defined in the middle of the event handlers.
While it will work correctly, I would like this to be defined together with the other helper functions, at the end of the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also why did you create a helper function for a native function that has exactly the same function signature as the abstraction?

This looks analogous to having a function called my_alert where inside you just call alert with the exact same parameters of my_alert:

function my_alert(msg) {
  alert(msg)
}

For the final confirmation box we'll do something very different from this so there is no reason to "prepare" for it like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also why did you create a helper function for a native function that has exactly the same function signature as the abstraction?
I thought we wanted to hook into it at some point, that's why I brought it outside.
I'll make the changes later this evening, thanks you

@ubidefeo ubidefeo closed this Jan 18, 2024
@ubidefeo ubidefeo force-pushed the feature/copy-delete-confirmation branch from 6572ffc to 6bdcff0 Compare January 18, 2024 21:30
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