-
Notifications
You must be signed in to change notification settings - Fork 931
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
Proposal to have optional confirmation dialog on row delete #665
Comments
So, I think adding dialogs for things like this takes us a bit out of the way of the intent of the library, which is to provide a (growing) set of core functionality that can be extended to the user's taste, but to stop short of specific implementations. For example, this library doesn't provide an export to excel option, but it allows for the toolbar to be customized such that one could be provided. Confirmation dialogs cross that line for me, where the devs have all of the tools necessary to provide this if they so choose, via custom rendering options and callbacks. If it were not possible to allow the right kind of customization, that would be a set of functionality I would support (the customization options that would lead to the capacity to implement confirmation dialogs). I also want to be careful of adding features which themselves would call for a large amount of customization down the road, as people will want options to change the dialog message, dialog appearance, type of modal, etc. I think it's much better to follow material ui's lead here and allow users of the library to implement their own. Thank you for your interest, and for asking. I think at some point, it might make sense to allow for some kind of plugin system, which would enable others to take up the mantle of providing these types of specific features, which could then be imported by others and dropped into the library, but I think it's a ways away from that at the moment. |
@gabrielliwerant We were also interested in adding a confirmation dialog to the onRowsDelete function, but ran into similar issues. One possible solution would be to allow onRowsDelete to be an async function. This would allow us to do logic regarding the delete, and return true or false from onRowsDelete when we actually know if the row has been deleted or not. |
@gabrielliwerant I've made a PR that allows an async onRowsDelete. This shouldn't affect functionality if onRowsDelete isn't async. #835 |
@pkmnct Hm, I hadn't considered this in terms of async, as that seems like a good solution. It's a little bit of a blurry line in that, if you really need a lot of custom functionality with delete, it's probably better to supply the entire delete functionality yourself via |
@gabrielliwerant would it be possible to add to the list of examples/demos one showing how to implement this specific functionality? |
I just finished implementing delete with a confirmation dialog - which I think is still considered best practice - and it was a bit clunky with the current implementation of
onRowsDelete
.I had to grab the selected elements and return false from
onRowsDelete
, then fire the dialog and on confirmation, pass the selected indexes up to the controller via callback for removal.Would you be open to a pull request that would allow the user to either request or provide a confirmation dialog on delete?
The text was updated successfully, but these errors were encountered: