Move the GraphVisualization wrapper#275
Conversation
| "scripts": { | ||
| "build": "webpack", | ||
| "postbuild": "cp dist/base.js ../python-wrapper/src/neo4j_viz/resources/nvl_entrypoint" | ||
| "dev": "concurrently -n widget,html,jupyter -c cyan,magenta,green \"npm:dev:widget\" \"npm:dev:html\" \"npm:dev:jupyter\"", |
There was a problem hiding this comment.
Convenience script to make developing faster/easier with hot reloading (no need to reload the kernel to see changes in javascript)
There was a problem hiding this comment.
nice 👍
could mention this in the Contributing.md
OskarDamkjaer
changed the title
FlorentinD
left a comment
FlorentinD
left a comment
There was a problem hiding this comment.
Thanks for your PR!
I think we can reduce the breaking changes a bit.
Couple of suggestions and questions. For the python parts, I can also try to apply some of them.
I would also put a link to https://storybook-components-build.appspot.com/?path=/story/components-graph-visualization--default for reference in the graph-widget.tsx file.
I think before merging the PR, we should change:
[ ] default coloring changed (fixable by setting the color in python if not set initially)
[ ] Zoom in/out button missing (fixable)
On the zoom in/out, i dont get yet why the storybook example has the buttons but its not referenced in the code
| // Only include visual properties when explicitly set, so that | ||
| // GraphVisualization's smart defaults (label-based coloring, etc.) apply. | ||
| ...(node.color !== undefined && { color: node.color }), | ||
| ...(node.size !== undefined && { size: node.size }), |
There was a problem hiding this comment.
are there docs around which are the smart defaults?
| id: node.id, | ||
| // Only include visual properties when explicitly set, so that | ||
| // GraphVisualization's smart defaults (label-based coloring, etc.) apply. | ||
| ...(node.color !== undefined && { color: node.color }), |
There was a problem hiding this comment.
compared to before - how can users now select one color for all nodes.
They could fix the color for each node
| ...(node.color !== undefined && { color: node.color }), | ||
| ...(node.size !== undefined && { size: node.size }), | ||
| ...(node.pinned !== undefined && { pinned: node.pinned }), | ||
| labels: Array.isArray(labels) |
There was a problem hiding this comment.
are the labels used for the results overview?
asking for when nodes/relationships are colored differently, such as by size or a property.
right now after VG.color_nodes(field="size"), the overview still shows the coloring based on the labels.
this is okay for now, we didnt have any overview before.
But for the future, maybe we could provide a hint for the colors in the overview?
| _STATIC = pathlib.Path(__file__).parent / "resources" / "nvl_entrypoint" | ||
|
|
||
|
|
||
| class GraphWidget(anywidget.AnyWidget): |
There was a problem hiding this comment.
i like the options this will give us.
might want to allow some utility methods in the future directly on the widget :)
| nodes: list[dict[str, Any]] = traitlets.List([]).tag(sync=True) # type: ignore[assignment] | ||
| relationships: list[dict[str, Any]] = traitlets.List([]).tag(sync=True) # type: ignore[assignment] |
There was a problem hiding this comment.
how about we use typed Node and relationship here, but specify a custom to_json method?
example from Anywidget
my_path = traitlets.Instance(pathlib.Path).tag(
sync=True, to_json=_serialize_entity
)
| # Simulate adding a node (as JS or Python might do) | ||
| widget.nodes = [*widget.nodes, {"id": "n2", "caption": "B"}] | ||
| assert len(widget.nodes) == 2 | ||
| assert widget.nodes[1]["id"] == "n2" |
There was a problem hiding this comment.
thats quite neat.
would make a good addition for an example notebook
Co-authored-by: Florentin Dörre <florentin@owitsch.de>
2 participants
I found that using anywidget makes the interaction with React /javascript much cleaner and less hacky. To avoid breaking the public API, render is still there return creating HTML, which i implemented by creating a tiny anywidget wrapper in plain html
I also added a new render_widget is there as a new alternative for 2 way data binding with the visualisation, as well as a GraphWidget export.
The PR is quite large 😅 and I've annotated best I can where I'm uncertain.
There's some open questions that'd be good to discuss:
python-wrapper&js-applet) It seem cleaner & simpler to me, but it was a bigger change than what I wanted to do without discussing it first