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

New: Add zIndex style property #53

Merged
merged 2 commits into from
May 12, 2023

Conversation

tonilastre
Copy link
Contributor

Additional zIndex property on nodes and edges that act similar to CSS z-index. If set on any of the nodes, the zIndex property value will be used as a stack order when rendering nodes/edges.

An example "Custom styled graph" has been updated where the first node has the highest zIndex value.

Screenshot 2023-05-01 at 12 25 16

@tonilastre tonilastre requested a review from cizl as a code owner May 1, 2023 10:37
@tonilastre tonilastre changed the base branch from main to release/0.4.0 May 3, 2023 16:26
Comment on lines 135 to 136
this.drawObjects(sortObjectsByZIndex(graph.getEdges()));
this.drawObjects(sortObjectsByZIndex(graph.getNodes()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this expensive? Can we keep the ordering contained inside the data structure for the nodes and edges?
Because this function gets called for every render cycle and if there are a bunch of elements this seems costly.
It seems to me that it would be wider to call a reorder function when we update the graph, rather than on every render cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct, it can be expensive (for small graphs not really), but it definitely doesn't make sense to sort it on each render.

The problem with the sorted structure is that it is hard to do with the current implementation because you can do node.properties.zIndex = 10 and there is no way for us to know that something changed. This is what we intended for the v1.0.0 where all setters would be setProperties() or setData() on nodes/edges so we can track changes and do reactions on top of it - it could be even a great use case for RxJS because we want to track multiple changes but throttle render depending on the defined FPS.

Currently, because of the above problem, the only way I can resolve this is by having a check if nodes/edges are sorted by zIndex, if yes, ok, if not, sort it within the graph instance. This way, the sorting happens only once but it will be checked on each render, unfortunately.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with leaving it like it is for now, but definitely write it down and take this into consideration when refactoring for v1.0.

Allowing the user to directly access the props is proving more and more to be tricky to handle.

Copy link
Contributor

Choose a reason for hiding this comment

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

So for now I'd just plop in a TODO here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added EntityModel and changed the code in such a way that ordering will happen on one of the following events:

  • data.setup
  • data.merge
  • data.remove
  • setDefaultStyle

And when we add reactivity, we can then refactor all of these to be more reactive on each node/edge style changes.

Copy link
Contributor

@cizl cizl left a comment

Choose a reason for hiding this comment

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

Amazing!

I think the entity stuff is a great improvement.

@tonilastre tonilastre merged commit 4800424 into release/0.4.0 May 12, 2023
@tonilastre tonilastre deleted the new/add-z-index-style-property branch May 12, 2023 08:47
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.

2 participants