search

cytoscape / cytoscape.js-compound-drag-and-drop

Compound node drag-and-drop UI for adding and removing children
MIT License
33 stars 16 forks source link

feat: allow orphaned parents #17

Closed sashokbg closed 2 years ago

sashokbg commented 2 years ago

There is a use case where it is desired to use "real" nodes as parent nodes instead of creating temporary ones.

This can be accomplished with "newParentNode" by returning the dropTarget: 

  newParentNode: (grabbedNode: any, dropTarget: any) => {
    return dropTarget;
  },

The new part is the addition of "allowOrphanedParents" which prevents the parent node from being deleted and allows the last child of a parent to be taken out.

maxkfranz commented 2 years ago

(1) It looks like your use of newParentNode() is based on a side-effect rather than by design (i.e. the return value is expected to be JSON, not an existing element). If this is to be allowed, then the documentation should be updated to make this use case explicit.

(2) Is a new option necessary if the existing newParentNode() function would be allowed to work as you propose? Could this be made automatic? The only reason the new parent would be removed when dragging one orphan over another is because no parent exists. This new parent has no reason for being if the gesture is cancelled, and so it is removed. This does not hold in your case, so I suspect you could just detect that case and then handle it appropriately.

sashokbg commented 2 years ago

Hello @maxkfranz,

I think that if we do not specify a "newParentNode" function, by default the target node should become a parent node - it feels natural.

If we accept this then the "allowOrphanedParents" can be removed and automated:

What do you think ?

maxkfranz commented 2 years ago

I don't think we should be changing the defaults just now. My main concern is that the consumer should not be specifying options that can be determined implicitly and internally by the library.

So internally, you can have something like allowOrphanedParents if that’s how you want to structure it (e.g. as a function or computed property), but it should be determined by the extension by inspecting the result of the existing options like newParentNode

On Nov 12, 2021, at 18:45, Alexander KIRILOV @.***> wrote:

Hello,

I think that if we do not specify a "newParentNode" function, by default the target node should become a parent node - it feels natural.

If we accept this then the "allowOrphanedParents" can be removed and automated:

If the parent node is a "real node" then always keep it and allow child nodes to exit it If the parent node is provided from "newParentNode" then remove parent nodes (working as previous) What do you think ?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cytoscape/cytoscape.js-compound-drag-and-drop/pull/17#issuecomment-967731116, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHRO44TNKJZKOFYFSTAO4TULWRKJANCNFSM5HDPWFDA.

sashokbg commented 2 years ago

closed in favor of #22 which takes into account your suggestion for automatic detection of real parents