search

iVis-at-Bilkent / cytoscape.js-context-menus

A Cytoscape.js extension to provide context menu around elements and core instance
MIT License
86 stars 41 forks source link

Append context menu to cytoscape container instead of body #51

Closed rparent closed 4 years ago

rparent commented 4 years ago

This is a backport of #50 on the right branch and without jQuery.

I have built and commited the new version, I don't know if you want to build for each PR or only at releases ?

hasanbalci commented 4 years ago

@rparent @msalihaltun I noticed a side affect of this PR. For example, in the demo, if we choose "select all nodes" from the root menu, all nodes are chosen as normal. Then if we choose "select all edges", all edges are chosen but all nodes are unselected unexpectedly. In the current unstable branch, all nodes stay as selected. I think, since context-menu is now appended to cy container, cy detects a click on itself and unselects the nodes. I played with z-index values of context-menus to prevent this behavior but couldn't manage to fix it.

msalihaltun commented 4 years ago

I think, since context-menu is now appended to cy container, cy detects a click on itself and unselects the nodes. I played with z-index values of context-menus to prevent this behavior but couldn't manage to fix it.

@hasanbalci @rparent In this case, maybe preventing bubbling on the menu item click events with preventDefault() can solve it by stopping the cy container from detecting the click event on itself.

rparent commented 4 years ago

@hasanbalci @msalihaltun I just pushed a commit to prevent mouse/touch events from being propagated when they occur inside the context menu, it seems to fix those unintended click issues. Does it fix your demo ?

msalihaltun commented 4 years ago

@rparent @hasanbalci Seems fixed now for me. Will test with Newt soon.

hasanbalci commented 4 years ago

@rparent I also tried in the demo and it's working correctly now, thanks!

msalihaltun commented 4 years ago

@hasanbalci Works correctly in Newt after the changes as well. I think this can be merged now.

rparent commented 4 years ago

@hasanbalci @msalihaltun Thanks for the merge !

Just to know, do you have a rough idea of the delay before this gets released ?

msalihaltun commented 4 years ago

@rparent Thank you for your contribution. We are planning to have a new release of this extension soon that should include this change.