Directed graph with viewGenerator #396

lironhl · 2020-11-14T20:37:19Z

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

src/components/marker/marker.helper.js

src/components/graph/graph.helper.js

antoninklopp · 2020-11-15T13:42:46Z

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

Links are not working anymore on the custom-node sandbox example

lironhl · 2020-11-15T19:01:23Z

@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

danielcaldas

Looking good, overall! Just a few comments to address, and we should be good to go 👍

src/components/graph/graph.const.js

src/components/graph/graph.helper.js

lironhl · 2020-11-28T13:12:45Z

@danielcaldas @antoninklopp Thank you both for the quality code review.

antoninklopp

Hi @lironhl ,

I think that this is almost ready to merge even if some questions need to be addressed.

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?

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.

Thanks for these awesome changes !

antoninklopp · 2020-11-29T10:05:30Z

src/components/graph/graph.helper.js

+ * @param {Object.<string, number>} VectorOriginCoords The center of the rectangle coords.
+ * @param {Object.<string, number>} directionVector a 2D vector with x and y components
+ */
+function calcRectangleVectorLengthFromCoords({ x1, y1, x2, y2 }, { px, py }, directionVector) {


Thi seems like redundant information since you can compute

let px = (x1 + x2)/2; let py = (y1 + y2)/2

Maybe remove?

If we assume that px and py represent the center of the Square, x and y match our needs, in this library x and y represent the center of a symbol (i.e. square, circle, wye), and not it's top left point.

If I am only looking at the data you are inputing into your method : (graph.coords.js, line 90):

const leftSquareX = x - edgeSize / 2; const topSquareY = y - edgeSize / 2; { x1: leftSquareX, y1: topSquareY, x2: leftSquareX + edgeSize, y2: topSquareY + edgeSize }

So here
(x1 + x2)/2 = (leftSquareX + leftSquareX + edgeSize)/2 = (2 * x - edgeSize + edgeSize)/2 = x
and
(y1 + y2)/2 = (topSquareY + topSquareY+ edgeSize)/2 = (2 * y - edgeSize + edgeSize)/2 = y

And it seems to be the same calculation when you call the method the other time.
Strictly from a parameters/data point of view, I feel like it is redundant information (since you are only calling this method for square/rectangle)
Unless you plan on calling it somewhere else?

src/components/graph/graph.helper.js

antoninklopp · 2020-11-29T10:41:53Z

src/const.js

@@ -12,5 +12,6 @@ export default {
    STAR: "star",
    TRIANGLE: "triangle",
    WYE: "wye",
+    RECTANGLE: "rectangle",


You are introducing a new type of symbol that was precedently not supported by react-d3-graph. This should maybe not be in this Pull Request but in another like Introducing Rectangle symbol since it adds a new feature unsupported before. What do you think?

I think that it also asks the question of whether the square symbol is still needed since a square is only a rectangle with four equal straight sides.

You are introducing a new type of symbol that was precedently not supported by react-d3-graph. This should maybe not be in this Pull Request but in another like Introducing Rectangle symbol since it adds a new feature unsupported before. What do you think?

That's a good point, but it's ok to ship the new RECTANGLE type in this PR, no need to separate it since our documentation will make that separation clear once we release a new version.

I think that it also asks the question of whether the square symbol is still needed since a square is only a rectangle with four equal straight sides.

Makes total sense! Hey @lironhl do you mind going ahead and add a @deprecated on the square here? You can write a note saying that we can deprecate the type "square" in a next major version release.

Hmmm, this is a difficult decision, I introduced the rectangle symbolType in order to be more explicit about the desired node shape, and to remind people to pass { width, height } object to the size property of the Node, but I do not intend to support rectangle SVG shape (I looked into it briefly and couldn't find easy solution).

I think i'll go in @LonelyPrincess way, when she implemented viewGenerator property, in case the user passes the { width, height } object to the Nodes size property I'll consider that node as a rectangle, because an { width, height } object do not mean anything when considering any other shape.

Allowing rectangle as a valid value in the symbolType property may create confusion because users will think it's also supported as an SVG shape

Yes but I am afraid that looking at the code we will have the impression that this symbol can be used when it is just there as a supported type for other purposes. I think that either way what you are choosing (supporting it or not), this should be clearer with what can and can not be done with this new type (maybe a clearer name like RECTANGLE_INTERNAL to show that it can not be used by the users)?

I see. I think I understand the dilemma now. @antoninklopp raised a good point though, how about making it so that the new type is used internally only. We could even rename it to VIEW_GENERATOR_CROP or something similar, in ways that it wouldn't be confused with a geometrical symbol type.

lironhl · 2020-12-04T22:09:33Z

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).

lironhl · 2020-12-05T18:22:23Z

@antoninklopp @danielcaldas This is no longer in a draft status for me.

src/components/graph/graph.coords.js

lironhl · 2021-02-06T11:44:53Z

@danielcaldas any idea why the node@10 test fails?

danielcaldas · 2021-02-12T07:02:14Z

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.

antoninklopp · 2021-02-16T19:34:22Z

@danielcaldas any idea why the node@10 test fails?

Hi @lironhl ,

I am not seeing any check failing. Which one are you referring to?

lironhl · 2021-02-16T19:36:48Z

@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?

antoninklopp

Sorry for the late review
Great work @lironhl
Thanks!

I added a few comments about the documentation but everything seems good on my side when I tested. However, there are really a lot of changes, I tried to look at everything carefully but hope I did not miss anything.

src/components/graph/graph.coords.js

antoninklopp · 2021-02-27T21:28:31Z

src/components/graph/graph.coords.js

+ */
+function calcVectorLength(symbolType, nodeSize, { x, y }, directionVector, isCustomNode) {
+  if (typeof nodeSize === "object" && nodeSize?.width && nodeSize?.height) {
+    return calcRectangleVectorLength(nodeSize, { x, y }, directionVector);


This seems strange that we are hacking the symbol type whenever a width and height are provided. What do you think?

Size as an object, currently only makes sense when a viewGenerator is provided.
This issue predates this PR.
Do you think a special SYMBOL that only supported with viewGenerator is the answer?

henri-edinb · 2021-03-11T19:37:20Z

hi, how do i incorporate this into the graph config?

danielcaldas · 2021-03-18T11:48:59Z

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!

lironhl · 2021-03-19T16:21:47Z

@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!

antoninklopp · 2021-04-03T13:49:08Z

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!

antoninklopp · 2021-05-13T13:51:22Z

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 !

GraemeF · 2021-08-09T11:34:36Z

This looks great! Hoping it can be merged 😃

…feature/rectangle_directed_graph_support

lironhl · 2021-08-09T13:03:18Z

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?

lironhl · 2021-08-09T22:26:17Z

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?

fortune-cook1e · 2022-12-09T08:55:26Z

@danielcaldas excuse me. When do you plan to merge this pr.

lironhl force-pushed the feature/rectangle_directed_graph_support branch 2 times, most recently from 07e0fb2 to a6edc59 Compare November 14, 2020 21:20

antoninklopp reviewed Nov 15, 2020

View reviewed changes

src/components/marker/marker.helper.js Outdated Show resolved Hide resolved

antoninklopp reviewed Nov 15, 2020

View reviewed changes

src/components/graph/graph.helper.js Outdated Show resolved Hide resolved

antoninklopp reviewed Nov 15, 2020

View reviewed changes

src/components/graph/graph.helper.js Outdated Show resolved Hide resolved

antoninklopp reviewed Nov 15, 2020

View reviewed changes

danielcaldas added the priority high label Nov 20, 2020

danielcaldas self-requested a review November 20, 2020 10:40

danielcaldas requested changes Nov 23, 2020

View reviewed changes

lironhl force-pushed the feature/rectangle_directed_graph_support branch from a6edc59 to 9c93a78 Compare November 28, 2020 13:11

lironhl requested review from danielcaldas and antoninklopp November 28, 2020 13:11

lironhl force-pushed the feature/rectangle_directed_graph_support branch 2 times, most recently from 44e9b8d to 23f4c1d Compare November 28, 2020 13:16

antoninklopp requested changes Nov 29, 2020

View reviewed changes

lironhl force-pushed the feature/rectangle_directed_graph_support branch 2 times, most recently from 0e994b0 to 95962c8 Compare December 4, 2020 20:06

lironhl force-pushed the feature/rectangle_directed_graph_support branch 3 times, most recently from 38eba75 to 6062367 Compare December 5, 2020 15:57

lironhl requested a review from antoninklopp December 5, 2020 16:01

lironhl force-pushed the feature/rectangle_directed_graph_support branch 3 times, most recently from fecd8e7 to 693bedf Compare December 5, 2020 18:16

lironhl changed the title ~~Draft: directed graph with viewGenerator~~ Directed graph with viewGenerator Dec 5, 2020

danielcaldas requested changes Jan 12, 2021

View reviewed changes

src/components/graph/graph.coords.js Outdated Show resolved Hide resolved

lironhl force-pushed the feature/rectangle_directed_graph_support branch 2 times, most recently from f7d2496 to 1534521 Compare February 6, 2021 11:31

lironhl requested a review from danielcaldas February 6, 2021 11:36

antoninklopp approved these changes Feb 27, 2021

View reviewed changes

lironhl force-pushed the feature/rectangle_directed_graph_support branch from 7ef82dd to efbbb53 Compare April 2, 2021 13:01

lironhl requested a review from antoninklopp April 2, 2021 13:02

lironhl force-pushed the feature/rectangle_directed_graph_support branch from efbbb53 to 6223748 Compare April 2, 2021 13:04

directed graph with viewGenerator

75797b9

lironhl force-pushed the feature/rectangle_directed_graph_support branch 2 times, most recently from eb7ef20 to 235aa10 Compare April 2, 2021 13:46

Documentation Improvments

372d064

lironhl force-pushed the feature/rectangle_directed_graph_support branch 2 times, most recently from 6001fd7 to 34bcc92 Compare April 2, 2021 13:54

Fix tests

bc4fdd2

lironhl force-pushed the feature/rectangle_directed_graph_support branch from 34bcc92 to bc4fdd2 Compare April 2, 2021 14:01

Merge branch 'master' of github.com:danielcaldas/react-d3-graph into …

c6302b4

…feature/rectangle_directed_graph_support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directed graph with viewGenerator #396

Directed graph with viewGenerator #396

lironhl commented Nov 14, 2020

antoninklopp commented Nov 15, 2020 •

edited

Loading

antoninklopp left a comment

lironhl commented Nov 15, 2020

danielcaldas left a comment

lironhl commented Nov 28, 2020

antoninklopp left a comment

antoninklopp Nov 29, 2020

lironhl Dec 4, 2020

antoninklopp Dec 5, 2020

antoninklopp Nov 29, 2020

danielcaldas Nov 30, 2020

lironhl Dec 4, 2020

antoninklopp Dec 5, 2020

danielcaldas Dec 17, 2020

lironhl commented Dec 4, 2020

lironhl commented Dec 5, 2020

lironhl commented Feb 6, 2021

danielcaldas commented Feb 12, 2021

antoninklopp commented Feb 16, 2021

lironhl commented Feb 16, 2021

antoninklopp left a comment

antoninklopp Feb 27, 2021

lironhl Apr 2, 2021

henri-edinb commented Mar 11, 2021

danielcaldas commented Mar 18, 2021

lironhl commented Mar 19, 2021

antoninklopp commented Apr 3, 2021

antoninklopp commented May 13, 2021

GraemeF commented Aug 9, 2021

lironhl commented Aug 9, 2021 •

edited

Loading

lironhl commented Aug 9, 2021

fortune-cook1e commented Dec 9, 2022

Directed graph with viewGenerator #396

Are you sure you want to change the base?

Directed graph with viewGenerator #396

Conversation

lironhl commented Nov 14, 2020

Circle

Square

Rectangle

antoninklopp commented Nov 15, 2020 • edited Loading

antoninklopp left a comment

Choose a reason for hiding this comment

lironhl commented Nov 15, 2020

danielcaldas left a comment

Choose a reason for hiding this comment

lironhl commented Nov 28, 2020

antoninklopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lironhl commented Dec 4, 2020

lironhl commented Dec 5, 2020

lironhl commented Feb 6, 2021

danielcaldas commented Feb 12, 2021

antoninklopp commented Feb 16, 2021

lironhl commented Feb 16, 2021

antoninklopp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henri-edinb commented Mar 11, 2021

danielcaldas commented Mar 18, 2021

lironhl commented Mar 19, 2021

antoninklopp commented Apr 3, 2021

antoninklopp commented May 13, 2021

GraemeF commented Aug 9, 2021

lironhl commented Aug 9, 2021 • edited Loading

lironhl commented Aug 9, 2021

fortune-cook1e commented Dec 9, 2022

antoninklopp commented Nov 15, 2020 •

edited

Loading

lironhl commented Aug 9, 2021 •

edited

Loading