Conversation
There was a problem hiding this comment.
Are useDeleteWithConfirmController and useDeleteWithUndoController deprecated? I'm not sure that it is a good choice as we can see that it forces UI implementation to duplicate parts of the logic (that will most likely be duplicated again in other UI kits). A good compromise could be to provide a few helpers / primitive functions with consistent side effects (like the propagation stop on click).
| ); | ||
| record && unselect([record.id]); | ||
| redirect(redirectTo, resource); | ||
| }, |
There was a problem hiding this comment.
It's sad to have this duplicated again. Maybe introduce a defaultOnSuccess / defaultOnError in useDeleteController so you can use them here?
There was a problem hiding this comment.
As discussed:
- both
useDeleteWithConfirmControlleranduseDeleteWithUndoControllerare deprecated - we don't want to export default side effects to reuse them
| if (typeof onClick === 'function') { | ||
| onClick(event); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This handleDelete override is exactly the same in useDeleteWithUndoController. What about doing it in useDeleteController directly? Or you want to keep useDeleteController as generic as possible so it can be used for something else than a clickable component?
There was a problem hiding this comment.
We want to keep useDeleteController as generic as possible so it can be used for something else than a clickable component
| ); | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Using something like defaultOnSuccess / defaultOnError like my other suggestion could help to deduplicate this code.
There was a problem hiding this comment.
As discussed, we'll keep the duplication
Problem
We currently have a lot of duplicated logic in
useDeleteWithConfirmControlleranduseDeleteWithUndoController. Besides they handle things that should only belong in the UI hooks/components (onClick, dialog handlers and state).Solution
useDeleteControllerthat can be used for all mutation modesuseDeleteWithConfirmControlleranduseDeleteWithUndoControlleras we want to keep them for backward compatibilityDeleteWithUndoButtonandDeleteWithConfirmButtonDeleteWithConfirmButtonso that it always closes the confirmation dialog before running user provided side effectsHow To Test
Additional Checks
masterfor a bugfix or a documentation fix, ornextfor a feature