farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
59 stars 38 forks source link

Move delete dialog to App.vue #459

Closed jgaehring closed 2 years ago

jgaehring commented 2 years ago

Right now the delete dialog and its associated state and methods are in Tasks.vue. This seems like a good feature to move to core, since the behavior should be consistent across Field Modules. I'd like to first move the markup, state, methods, and custom events to App.vue, then work out a way to embed this process directly into the delete Vuex actions, so the events don't have to be fired manually with each corresponding deletion. It would also be good UX, imo, to enforce this warning for all delete actions.

A corollary to this issue is having some kind of general handling of modals in App.vue, but that can be developed later.

jgaehring commented 2 years ago

See https://github.com/farmOS/farmOS-client/issues/190#issuecomment-927847037.

It might be better to just wait and treat this along with merge conflicts, as well as error handling (#428) for that matter, all beta milestone issues.

jgaehring commented 2 years ago

As I mentioned in the comments of the AppModal.vue, via c30cd8e:

This is just a fast & cheap way of temporarily stashing the delete dialog, previously embedded in the Tasks container component, here in AppModal, which itself is no longer needed since the drawer has its own implementation, as of the previous commit. Eventually, this should become a special component, akin to AppBar or AppDrawer, whose state is managed by AppShell, which itself can provide a method to all field modules for triggering a modal dialog. One instance of this will be the delete dialog, which AppShell can trigger auto- matically whenever a deletion is performed.

That should resolve the main concerns of this issue, namely decoupling that logic from the Tasks module. The remaining concerns around deleting entities can be tracked in #411, primarily, and to a lesser extent in #284.