Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding support for context-menu #196

Merged
merged 50 commits into from
Nov 23, 2023
Merged

adding support for context-menu #196

merged 50 commits into from
Nov 23, 2023

Conversation

cleaaum
Copy link
Contributor

@cleaaum cleaaum commented Oct 20, 2023

This PR integrates the external js library "cytoscape-context-menus" to give the user the ability to define its own context menu.

In order to add a context menu item the user needs to use the contextMenu prop and pass at least the following items to create the menu item.

contextMenu=[
                {
                    "id": "item-id",
                    "label": "menu item text",
                    "tooltipText": "tooltip",
                    "availableOn": ["node", "edge"],
                    "onClick(Custom)": "behaviour",
                }
]

Then, the user has three options to define the onClick behaviour:

  1. Using a pre defined js function by passing one of the built-in functions. For now, remove, add_node, and add_edge are supported. This is done by setting "onClick": "remove" for example.
  2. Using a namespace by providing a user defined js function in window.dashCytoscapeComponentFunctions. This is done by setting "onClickCustom": "add-2-nodes" for example, where add-2-nodes is defined in the namespace.
  3. Using Python in a Dash Callback. In this case js will return relevant information to modify the Cytoscape elements accordingly. These are accessible through the ContextMenuData and elements prop. The user must simply omit the "onClick" key.

I've provided a sample app (usage-context-menu.py) with a context menu and also provided a js namespace under assets/ctxMenuFns.js with an example of a user defined js function.
This example has the following context menu items implemented via the following methods:

  1. Using a pre defined js function:remove, add_node, and add_edge.
  2. Using a namespace: add-2-nodes
  3. Using Python in a Dash Callback: split_edge and revert_edge

Pre-Merge checklist

  • The project was correctly built with npm run build:all.
  • If there was any conflict, it was solved correctly.
  • All changes were documented in CHANGELOG.md.
  • All tests on CircleCI have passed.
  • All Percy visual changes have been approved.
  • Two people have 💃'd the pull request. You can be one of these people if you are a Dash Cytoscape core contributor.

@alexcjohnson
Copy link
Collaborator

@cleaaum this looks great! Two questions about the API:

  • Does it need to be onClickFunction or could we shorten it to onClick?
  • I'm a little uneasy with distinguishing built-in functions from user-defined functions just by name. Makes it a little tricky to tell as you're reading the code whether it's a built-in or user-defined function, but also it means if we ever decide to add more built-in functions we risk breaking user code if they happen to already be using the name we add. That said I'm unsure what to suggest as an alternative. One option is to give the built-in functions names that are not valid names (@remove or *remove or at least not conventional names (REMOVE or __remove__). Another would be to use different keys for the built-in and user-defined functions - onClickUser vs onClickBuiltIn.

@cleaaum
Copy link
Contributor Author

cleaaum commented Nov 6, 2023

@alexcjohnson Thank you for the review Alex! I've addressed all comments except for the add-edge one (i've left an explanation). I think I prefer differentiating them with onClickUser and onClickBuiltIn, or even just onClick.

MANIFEST.in Outdated Show resolved Hide resolved
demos/usage-context-menu.py Outdated Show resolved Hide resolved
Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🚀 🚀

Looks good to me!!!

I notice there is still one failing CI step (let me know if you want another pair of eyes on that) but in my eyes this is essentially ready. Great work @cleaaum 💪

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Looks great!

@Farkites Farkites merged commit 58d88cf into master Nov 23, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants