react-d3-graph
react-d3-graph copied to clipboard
danielcaldas
Directed graph with viewGenerator
Closes #389
This PR creates the expected link behavior, when viewGenerator is used, it supports the symbols: square, circle and rectangle.
This PR is very liberal with code structure changes, waiting to work with you @danielcaldas.
This PR still needs a little bit of work:
- Tests.
- There's a bug involving the links when two nodes that are connected, are too close to each other.
Circle
Square
Rectangle
Hi @lironhl, @danielcaldas,
I allowed myself to drop a few comments since I worked on the circle version of this node calculation.
Amazing work on generalizing the concept on all symbol types!
@antoninklopp Thanks for the CR, will fix the issues in the weekend, just wanted to say I forgot to add a comment about the source of the formula I used to calculate the length of a vector inside a rectangle.
Here it's: https://stackoverflow.com/a/3197924
My biggest one is for custom node support. Should it be supported for now? Since it would be the user that should choose the symbol that is the closest to its custom node shape. I personally think that it would still be a good addition, but should maybe be discussed?
I think it's ok to leave the responsibility of choosing the correct node symbol to the user, in order for him to have optimized positioning (in case the user specifies other symbol, we fallback to the previous behaviour, this is also the main mission of this PR (directed graph with viewGenerator).
Below is a screenshot from the custom-node data where we expect a square node but the custom node does not exactly render as a square and causes some inaccurate pointing at one edge.
Yep, that was totally my responsibility to pick custom-node that's rectangle in shape, but I didn't care that much, I thought people could still understand the value of the PR, even though the custom-component is not exactly a rectangle (maybe I can change it's CSS).
I'll let @antoninklopp have a look at this one I think he's been involved in the PR discussions at a deeper level than me.
@danielcaldas any idea why the node@10 test fails?
Hi @lironhl ,
I am not seeing any check failing. Which one are you referring to?
@danielcaldas any idea why the node@10 test fails?
Hi @lironhl ,
I am not seeing any check failing. Which one are you referring to?
I thought some build failed, seems like I was wrong.
On another note, what do you think should happen, in order for us to merge this PR?
Hey @lironhl it appears there are some conflicts, it would be awesome if you could attend to those and the comments lefted by @antoninklopp. Thanks!
@danielcaldas I'll find time to fix the PR in the following couple of days, let's hope for a merge in the span of the next two weeks!
Hi @lironhl ,
Thank you very much for updating this PR !
Could you please fix the tests? I see that some are still not successful?
I will review again as soon as this is fixed.
Hope that we will be able to merge this soon !
Cheers!
Hi @lironhl ,
Did you have any opportunity to fix the failing tests?
I think that it would be really great if we could merge this !
Hi @lironhl ,
Did you have any opportunity to fix the failing tests?
I think that it would be really great if we could merge this !
@antoninklopp The tests seem to work now, can we merge this?
I'll let @antoninklopp have a look at this one I think he's been involved in the PR discussions at a deeper level than me.
@danielcaldas can you match @antoninklopp approval?
