codecombat / treema

jQuery plugin that generates HTML interfaces to edit JSON data defined by json-schema.
http://codecombat.github.io/treema/
MIT License
152 stars 36 forks source link

Add a checking callback before deleting a node. #18

Open gintau opened 10 years ago

gintau commented 10 years ago

Hello @sderickson,

According to CodeCombat #507, components should care about their dependencies before being deleted. Currently the deleting action is handled by treema, but dependencies are found by thang editor. So I'm thinking about adding a callback (passed by constructor) in onDeletePressed() or in removeSelectedNodes(). If the callback exists and return false, then treema should prevent deletion from proceeding. How do you think?

sderickson commented 10 years ago

I don't think the callbacks should affect behavior. Adding a delete callback could still work, though. I'd say:

  1. Have remove mark itself as changed.
  2. Have broadcastChanges also call a 'delete' callback, providing all nodes that were deleted.

Then in CodeCombat, we can have the editor get all the info it needs to also delete any other nodes which were dependent on the deleted node. How's that sound?

gintau commented 10 years ago

I agree that callbacks should not affect behavior, but there are still some issues to consider. One is that some errors are raised if components depended by others are deleted, which prevent editor from working correctly. By your mean, we can delete a component and its dependents, but the errors would still appear.

Now I'm trying to find a way to alert users and prevent them from deleting the components which has dependents. Of course, the best case is to do this without injecting something which affects default behavior.

sderickson commented 10 years ago

Hmm, good point. The only other solution I can think of that would prevent removal from happening would be to create an abstract method canRemove which can be overridden. But that's not much of a better solution. Okay, let's go with the callback and see how it goes as a system. I suppose the error will be shown through the Treema itself?