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

Node margins #3162

Open
4 tasks
danprince opened this issue Sep 12, 2023 · 9 comments
Open
4 tasks

Node margins #3162

danprince opened this issue Sep 12, 2023 · 9 comments
Labels
pinned A long-lived issue, such as a discussion

Comments

@danprince
Copy link

Description of new feature

What should the new feature do? For visual features, include an image/mockup of the expected output.

Add a margin property which allows users to control the spacing between nodes and edge connection points.

image

This would involve adding one new style property to Cytoscape, which would only apply to nodes.

  • margin the space to include between the outside edge of a node and its edge connection points

Considerations:

  • Should relative margins be supported (e.g. 25%)? How would they work on shapes with irregular aspect ratios?

Motivation for new feature

Describe your use case for this new feature.

This styling can't be achieved as part of an extension (although it can be hacked to some degree if you're happy to sacrifice having a border, which is a dealbreaker for us) because it involves adding new style properties and adjusting the calculations which are used to position edges.

Part of trying to achieve Kumu parity, from #3145.

For reviewers

Reviewers should ensure that the following tasks are carried out for incorporated issues:

  • Ensure that the reporter has adequately described their idea. If not, elicit more information about the use case. You should iteratively build a spec together.
  • Ensure that the issue is a good fit for the core library. Some things are best done in extensions (e.g. UI-related features that aren't style-related). Some things are best done by app authors themselves -- instead of in Cytoscape libraries.
  • The issue has been associated with a corresponding milestone.
  • The commits have been incorporated into the unstable branch via pull request. The corresponding pull request is cross-referenced.
@maxkfranz
Copy link
Member

maxkfranz commented Sep 13, 2023 via email

@danprince
Copy link
Author

How would this differ from the existing edge endpoint properties? Are there different use cases or edge cases?

The edge-endpoints properties do what we need, but we want to move the edge endpoints away consistently from a given set of nodes.

In Kumu we use :to(...) and :from(...) selectors, which select all edges to or from a given node selector. This would make these two rules functionally equivalent.

node[type=person] {
  margin: 10px;
}

:to(node[type=person]), :from(node[type=person]) {
  source-distance-from-node: 10px;
  target-distance-from-node: 10px;
}

I think the use-cases are pretty much the same: create more space and breathing room around nodes. In our opinion the user experience is more intuitive if you can control the spacing consistently based on the type of node.

The obvious edge case that I hadn't identified before is around how you handle conflicts between the two properties. I think that because the edge-endpoint properties are more granular, they should always override the margin properties.

@maxkfranz
Copy link
Member

This may motivate revisiting arrow selectors for edges, e.g. .foo -> .bar or node.foo -> node. That would remove conflicts arising from the addition of new properties: The existing edge properties would suffice so long as you can use selectors to specify edges relative to particular sorts of nodes.

The arrow selectors were deprecated for performance and other reasons, so if we go that route, then we ought to do a bit of analysis on the use cases and the trade-offs.

For traversals generally, it's best to avoid them in selectors. They can be very expensive, just as they are in CSS proper. They can be simulated by applying classes using the traversal API once a network has been loaded. For small networks, the performance may not matter and the convenience of selectors may win out. It depends on the use cases.

Ref:

@stale
Copy link

stale bot commented Oct 11, 2023

This issue has been automatically marked as stale, because it has not had activity within the past 14 days. It will be closed if no further activity occurs within the next 7 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

@stale stale bot added the stale label Oct 11, 2023
@stale stale bot closed this as completed Oct 18, 2023
@maxkfranz maxkfranz added pinned A long-lived issue, such as a discussion and removed stale labels Oct 18, 2023
@maxkfranz maxkfranz reopened this Oct 18, 2023
@maxkfranz
Copy link
Member

Pinned

@maxkfranz
Copy link
Member

TBD:

  • De-deprecate edge arrow selectors?
  • Allow node margins in addition to edge offsets/distances from bodes?
  • Precedence if both node margin and edge offset are specified? Always prefer one prop? Use the max of the values?

@maxkfranz
Copy link
Member

maxkfranz commented Oct 26, 2023

I'm currently leaning towards supporting both the edge offset properties and node margin properties. A simple approach that may feel intuitive to devs would be to be consistent with CSS: In CSS proper, when you have two adjacent elements with margins, the larger margin 'wins out'. So if we were to be consistent with that, then an edge with both an offset and a connected node margin would use the max(edge offset, node margin) when drawing the edge.

How does that sound, @danprince?

@danprince
Copy link
Author

Sounds great @maxkfranz, should be back into a usual working rhythm now. Should get a chance to land some PRs in the next couple of weeks.

@maxkfranz
Copy link
Member

Sounds good. Looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned A long-lived issue, such as a discussion
Projects
None yet
Development

No branches or pull requests

2 participants