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

Proposal to have optional confirmation dialog on row delete #665

Open
shawnmitchell opened this issue Jun 4, 2019 · 5 comments
Open

Proposal to have optional confirmation dialog on row delete #665

shawnmitchell opened this issue Jun 4, 2019 · 5 comments

Comments

@shawnmitchell
Copy link
Contributor

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?

@shawnmitchell shawnmitchell changed the title Proposal to pass confirmation dialog to row delete handler Proposal to have optional confirmation dialog to row delete Jun 4, 2019
@shawnmitchell shawnmitchell changed the title Proposal to have optional confirmation dialog to row delete Proposal to have optional confirmation dialog on row delete Jun 4, 2019
@gabrielliwerant
Copy link
Collaborator

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 gabrielliwerant added the wontfix Support for this request is not planned at this time label Jun 12, 2019
@pkmnct
Copy link
Contributor

pkmnct commented Aug 14, 2019

@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.

@pkmnct
Copy link
Contributor

pkmnct commented Aug 14, 2019

@gabrielliwerant I've made a PR that allows an async onRowsDelete. This shouldn't affect functionality if onRowsDelete isn't async. #835

@gabrielliwerant
Copy link
Collaborator

@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 customToolbarSelect. But I understand that's much more work, so this may be a good solution. Will take a look.

@gabrielliwerant gabrielliwerant added enhancement and removed wontfix Support for this request is not planned at this time labels Aug 14, 2019
@gsandorx
Copy link

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 would it be possible to add to the list of examples/demos one showing how to implement this specific functionality?

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