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

Configurable graph force strength and link length #104

Merged

Conversation

@LonelyPrincess
Copy link
Collaborator

LonelyPrincess commented Sep 4, 2018

The two following parameters can be now be customized through the config object:

  • link.size (the length of the link)
  • node.gravity (defines how close nodes are from each other in the graph)

Until now these two values were set as constants (FORCE_IDEAL_STRENGTH and LINK_IDEAL_DISTANCE), but the predefined values could present visual issues for bigger node sizes.

In some scenarios, the links wouldn't even appear on screen, as they would be completely hidden beneath the nodes. Also, it could happen that many of the nodes happened to overlap, making it impossible to view all of them without dragging them to isolated locations.

Having these properties as configurable values will make it easier for the developers to customize the distance between nodes depending on their size, and so ensure the graph looks good regardless of their choice.

The length of the links can be now configured via the config object. The specified length will include not only the visible segment of the link, but also the part hidden beneath the nodes it connects, which equals the half of each node' size.
Now it's posible to customize the graph force strength, which will allow
the developer to define the gravity between the nodes. The longer the
value, the closer they will be. If, on the contrary, a big negative
value is set, the nodes will be far from each other.
@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Sep 4, 2018

Good initiative, linking Trello card that corresponds to this PR, will review asap.

https://trello.com/c/NBdYTyV2

Copy link
Owner

danielcaldas left a comment

Nice one, still I think we should make a few small changes before merging this. Thanks!

@@ -97,6 +97,9 @@
*
* **[note]** react-d3-graph will map this values to [d3 symbols](https://github.com/d3/d3-shape#symbols)
* <br/>
* @param {number} [node.gravity=-100] - this will define how close nodes are to each other.

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Sep 8, 2018

Owner

I would like to see this values isolated in the scope of a d3 object property, can you please move this to:

...
d3: {
    gravity: -100,
    linkSize: 100
},
...
@@ -101,7 +101,7 @@ export default class Graph extends React.Component {

const forceLink = d3ForceLink(this.state.d3Links)
.id(l => l.id)
.distance(D3_CONST.LINK_IDEAL_DISTANCE)
.distance(this.state.config.link.size || D3_CONST.LINK_IDEAL_DISTANCE)
.strength(D3_CONST.FORCE_LINK_STRENGTH);

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Sep 8, 2018

Owner

What do you think of also having this constants configurable:

// Some d3 constant values
const D3_CONST = {
    FORCE_LINK_STRENGTH: 1,
    LINK_IDEAL_DISTANCE: 100,
    SIMULATION_ALPHA_TARGET: 0.05
};

They are on the top of this file

const simulation = _createForceSimulation(
newConfig.width,
newConfig.height,
newConfig.node && newConfig.node.gravity

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Sep 8, 2018

Owner

No need for node.gravity check this prop will always exist

The D3_CONST object has been removed, and all of the properties it had
inside are now configurable via the config object of the `Graph`
component.

All of these d3 related values are now wrapped in a new `d3` section
inside of the config object.
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Sep 8, 2018

Thanks for the feedback, @danielcaldas !

I've just uploaded another commit with the changes you proposed. Now both FORCE_LINK_STRENGTH and SIMULATION_ALPHA_TARGET are also customizable, and all of these new settings are wrapped into a d3 object within config.

Since they're no longer constants, I removed the whole D3_CONST object from Graph.jsx file as well. Their values are already set in the default config object, so there was no need to keep it any longer.

If there's something else you'd like me to change, don't hesitate to let me know. 😃

Copy link
Owner

danielcaldas left a comment

Only some small additional comments

src/components/graph/graph.helper.js Outdated Show resolved Hide resolved
@@ -360,7 +362,7 @@ function initializeGraphState({ data, id, config }, state) {
let links = _initializeLinks(graph.links); // matrix of graph connections
const { nodes: d3Nodes, links: d3Links } = graph;
const formatedId = id.replace(/ /g, '_');
const simulation = _createForceSimulation(newConfig.width, newConfig.height);
const simulation = _createForceSimulation(newConfig.width, newConfig.height, newConfig.d3.gravity);

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Sep 9, 2018

Owner

Can you please check whether some is wrong with this line? Tests for graph.helper are breaking in this property fetch, it seems that newConfig.d3 is undefined. Check this link for more detail.

This comment has been minimized.

Copy link
@LonelyPrincess

LonelyPrincess Sep 10, 2018

Author Collaborator

I've been taking a look at this and it seems that there are some tests in graph.helper.test.js where config is assigned a string instead of an actual config object. This is what's causing the error to occurr in some of them.

{ config: 'config' }

Since config is not an object at all in this case, newConfig.d3 is undefined, and the same happens with the other two parameters passed down to _createForceSimulation.

Fixing the issue is as simple as checking if newConfig.d3 before trying to access the gravity property.

const simulation = _createForceSimulation(newConfig.width, newConfig.height, newConfig.d3 && newConfig.d3.gravity);

I'll submit a commit with the change in a bit, but I'm curious... Is there any reason why config is taking a string value in these tests? 🤔

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Sep 10, 2018

Owner

Yes, so in those particular unit tests I was checking for API compatibility I wasn't particularly interested in checking whether config was a string or other data type.

@danielcaldas danielcaldas merged commit 2c11cd8 into danielcaldas:master Sep 10, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zvolchak zvolchak mentioned this pull request Sep 21, 2018
zvolchak added a commit to zvolchak/react-d3-graph that referenced this pull request Oct 29, 2018
* feat: Customize link length

The length of the links can be now configured via the config object. The specified length will include not only the visible segment of the link, but also the part hidden beneath the nodes it connects, which equals the half of each node' size.

* feat: Configure graph force strength

Now it's posible to customize the graph force strength, which will allow
the developer to define the gravity between the nodes. The longer the
value, the closer they will be. If, on the contrary, a big negative
value is set, the nodes will be far from each other.

* fix: Check if config.node exists before retrieving gravity

* feat: All values in D3_CONST are now configurable

The D3_CONST object has been removed, and all of the properties it had
inside are now configurable via the config object of the `Graph`
component.

All of these d3 related values are now wrapped in a new `d3` section
inside of the config object.

* refactor: Unneeded fallback line removed

* fix: Tests failing due to undefined newConfig.d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.