Feature/ondoubleclicknode #202

IgorMacGregor · 2019-05-15T07:28:52Z

Hi ! I have added the onDoubleClick feature as requested on issue #194. There is still a small bug that I couldn't fix: when double-clicking on a node, a zoom is triggered on the graph and I don't know how we could cancel it but it should be related to the d3-zoom library.
I've done my best to update the documentation, tell me if something is missing ! And it seems that some prettier script makes me change some of the README signs, I don't know how I could cancel it :'(
Have a good day !

danielcaldas · 2019-05-15T19:09:57Z

sandbox/Sandbox.jsx

@@ -64,6 +64,10 @@ export default class Sandbox extends React.Component {
        // });
    };

+    onDoubleClickNode = id => {
+        !this.state.config.collapsible && window.alert(`Double clicked node ${id}`);


Can you give it a try and write before the call add:

event.stopPropagation()

Note that first you need to grab the event in the parameters

I have done many attempts but it didn't work ... I've also tried preventDefault() but it doesnt do anything ...
I was thinking that maybe there would be a way to say to d3-zoom to zoom only if the double click happens on the graph ? As the onClickGraph function isn't triggered when clicking the node, it consider both clicks differently and we could leverage that ...

Currently, the only way I found to solve this event was disabling double-click zoom on the whole graph, by adding .on("dblclick.zoom", null) to the _zoomConfig function. I am trying to implement it only on the nodes but that doesn't work yet

danielcaldas

Thanks for the PR, please consider the following comments.

danielcaldas · 2019-05-15T19:58:28Z

README.md

@@ -20,9 +20,9 @@

 You can also load different datasets and configurations via URL query parameter, here are the links:

-*   [small dataset](https://goodguydaniel.com/react-d3-graph/sandbox/index.html?data=small) - small example.
-*   [custom node dataset](https://goodguydaniel.com/react-d3-graph/sandbox/index.html?data=custom-node) - sample config with custom views.


Did prettier ran against README.md 🤔

I think I found a way to correct that :)

danielcaldas · 2019-05-15T19:58:58Z

docs/DOCUMENTATION.md

@@ -2,94 +2,94 @@

 ### Table of Contents


Please discard the changes on this file 🙂

danielcaldas · 2019-05-15T19:59:52Z

package-lock.json

@@ -20,10 +20,10 @@
            "dev": true,


Please discard changes to this file 🙂

danielcaldas · 2019-05-15T20:00:42Z

src/components/graph/Graph.jsx

@@ -58,6 +58,10 @@ import utils from "../../utils";
 *      window.alert('Clicked node ${nodeId}');
 * };


Please also discard changes for src/components/graph/Graph.jsx

I don't understand this one, you mean I should delete the onDoubleClickNode function ?

Please ignore this one 😄

danielcaldas · 2019-05-15T20:02:20Z

src/components/graph/Graph.jsx

-            this.props.onClickNode && this.props.onClickNode(clickedNodeId);
+            if (!this.timeoutID) {
+                this.timeoutID = setTimeout(() => {
+                    this.props.onClickNode && this.props.onClickNode(clickedNodeId);


Is there any specific reason for this call to be wrapped inside a setTimeout?

Yes, it represents the time when the Node is waiting to see if another click is coming, and then decides if it should apply the onClickNode or onDoubleClickNode function

I don't know if you tried it out, but there's also a native dblclick event in JavaScript. Maybe you could try to use that instead of the setTimeout hack and see what happens.

If it doesn't work, this solution you came up with looks good to me. Still, since this is meant to be a handler for nodes only, I think it would be better to do this at node level instead of in the Graph component. Consider moving this logic to the handleOnClickNode method in Node component instead. 😉

For the dblclick event, it didn't work, JS and React as a whole have huge problems managing click and doubleclick on the same element and StackOverflow is full of questions regarding this issue.
I have run some tests to try to move the logic to the Node component but it doesn't work anymore, nothing is triggered when I double click, I don't understand why ...

danielcaldas · 2019-05-20T21:34:21Z

src/components/graph/Graph.jsx

@@ -293,7 +298,16 @@ export default class Graph extends React.Component {
                () => this.props.onClickNode && this.props.onClickNode(clickedNodeId)
            );
        } else {
-            this.props.onClickNode && this.props.onClickNode(clickedNodeId);
+            if (!this.timeoutID) {
+                this.timeoutID = setTimeout(() => {


I would prefer the naming this.nodeClickTimer. Also, do you need to assign the timer to this?

Yes, from what I've tried we can't initiate a variable (here, a nodeClickTimer not linked to this) in a component, or I would need to add a state but I found that too heavy for just one functionality

IgorMacGregor · 2019-05-27T10:57:34Z

I have disabled the double click zoom on the whole graph, I think it's the better (and only possible from what I know) way !
Sorry for the multiple commits, I had some difficulties to revert my change and I am rather new to Git ...
I have corrected the changes by adding docs/DOCUMENTATION.md and README.md on the .prettierignore file, so I suggest adding them on the master branch !
Thanks, have a good day :)

LonelyPrincess · 2019-05-31T21:48:00Z

src/components/graph/Graph.jsx

-            this.props.onClickNode && this.props.onClickNode(clickedNodeId);
+            if (!this.timeoutID) {
+                this.timeoutID = setTimeout(() => {
+                    this.props.onClickNode && this.props.onClickNode(clickedNodeId);


I don't know if you tried it out, but there's also a native dblclick event in JavaScript. Maybe you could try to use that instead of the setTimeout hack and see what happens.

If it doesn't work, this solution you came up with looks good to me. Still, since this is meant to be a handler for nodes only, I think it would be better to do this at node level instead of in the Graph component. Consider moving this logic to the handleOnClickNode method in Node component instead. 😉

LonelyPrincess · 2019-05-31T23:06:48Z

sandbox/Sandbox.jsx

@@ -64,6 +64,11 @@ export default class Sandbox extends React.Component {
        // });
    };

+    onDoubleClickNode = (event, id) => {
+        event.stopPropagation(); //??? Marche pas


I tried running the sandbox in your branch, but I'm getting the following error when I double click on a node:

Uncaught TypeError: event.stopPropagation is not a function

I debugged it a little and I found out that the first parameter of the method contains the node id instead of an event. The id parameter is always undefined, and no event is ever received by Sandbox's onDoubleClickNode function.

Although you added the event parameter in this function, you didn't add it in the Node and Graph components. The handleOnClickNode function in the Node component is the one that actually catches the event, and it must pass it to the parent components so it reaches the Sandbox callback. To begin with, we must pass the event to the Graph component like this:

handleOnClickNode = event => this.props.onClickNode && this.props.onClickNode(event, this.props.id);

After doing this, you have to also update the onClickNode function inside of the Graph component to receive the new event parameter and then update the call to this.props.onDoubleClickNode by passing it the event as the first parameter.

this.props.onDoubleClickNode(event, clickedNodeId)

Only when this is done you could actually try to call event methods from the Sandbox component. 🙂

This line was supposed to be deleted sorry, it is when I tried to cancel the zoom due to d3-zoom but it also led me to a dead-end, I couldn't cancel it

IgorMacGregor · 2019-06-18T09:13:21Z

Hello,
I have received a message saying that this PR is becoming stale, do you want any more changes ? Everything works now and I have followed your advices :)
Have a good day !

danielcaldas · 2019-06-20T20:48:32Z

Hey @IgorMacGregor thanks for noticing I will proceed with this soon 🙂

…lUnmount

IgorMacGregor added 5 commits May 15, 2019 11:34

Added onDoubleClickNode

05c2d37

corrected prettier changes

e475410

corrected prettier changes

b688ecb

corrected prettier changes

02884cc

corrected prettier changes

1aa7e35

danielcaldas reviewed May 15, 2019

View reviewed changes

danielcaldas requested changes May 15, 2019

View reviewed changes

danielcaldas reviewed May 20, 2019

View reviewed changes

IgorMacGregor added 4 commits May 21, 2019 20:29

Requested changes #1

2aa722f

Corrected README.md

59a7395

New Changes

f249b2c

Reverted DOCUMENTATION.md

34ccc33

danielcaldas requested a review from LonelyPrincess May 27, 2019 20:33

LonelyPrincess requested changes May 31, 2019

View reviewed changes

Deleted the forgottent line in Sandbox.jsx

ef39807

danielcaldas added 3 commits June 20, 2019 22:51

Add ttl for double click to const file. Clear timeout in componentWil…

b18d875

…lUnmount

Discard changes for package-lock.json and rd3g.sandbox.bundle.js

4fcb640

Add documentation on why double click on graph from d3 was disable

1d5087d

danielcaldas approved these changes Jun 20, 2019

View reviewed changes

danielcaldas merged commit 1161c61 into danielcaldas:master Jun 20, 2019

danielcaldas mentioned this pull request Mar 1, 2020

onDoubleClickNode handler never called when nodes are collapsible (onClickNode handler called twice instead) #294

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ondoubleclicknode #202

Feature/ondoubleclicknode #202

IgorMacGregor commented May 15, 2019

danielcaldas May 15, 2019

IgorMacGregor May 16, 2019

IgorMacGregor May 16, 2019

danielcaldas left a comment

danielcaldas May 15, 2019

IgorMacGregor May 16, 2019

danielcaldas May 15, 2019

danielcaldas May 15, 2019

danielcaldas May 15, 2019

IgorMacGregor May 16, 2019

danielcaldas May 27, 2019

danielcaldas May 15, 2019

IgorMacGregor May 16, 2019

LonelyPrincess May 31, 2019

IgorMacGregor Jun 3, 2019

danielcaldas May 20, 2019

IgorMacGregor May 21, 2019

IgorMacGregor commented May 27, 2019 •

edited

Loading

LonelyPrincess May 31, 2019

LonelyPrincess May 31, 2019 •

edited

Loading

IgorMacGregor Jun 3, 2019

IgorMacGregor commented Jun 18, 2019

danielcaldas commented Jun 20, 2019

		@@ -58,6 +58,10 @@ import utils from "../../utils";
		* window.alert('Clicked node ${nodeId}');
		* };

Feature/ondoubleclicknode #202

Feature/ondoubleclicknode #202

Conversation

IgorMacGregor commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcaldas 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

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

IgorMacGregor commented May 27, 2019 • edited Loading

Choose a reason for hiding this comment

LonelyPrincess May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMacGregor commented Jun 18, 2019

danielcaldas commented Jun 20, 2019

IgorMacGregor commented May 27, 2019 •

edited

Loading

LonelyPrincess May 31, 2019 •

edited

Loading