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

Feature/ondoubleclicknode #202

Merged
merged 13 commits into from
Jun 20, 2019
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ const onClickNode = function(nodeId) {
window.alert(`Clicked node ${nodeId}`);
};

const onDoubleClickNode = function(nodeId) {
window.alert(`Double clicked node ${nodeId}`);
};

const onRightClickNode = function(event, nodeId) {
window.alert(`Right clicked node ${nodeId}`);
};
Expand Down
5 changes: 5 additions & 0 deletions sandbox/Sandbox.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ export default class Sandbox extends React.Component {
// });
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

onRightClickNode = (event, id) => {
event.preventDefault();
window.alert(`RIGHT clicked node ${id}`);
Expand Down Expand Up @@ -317,6 +321,7 @@ export default class Sandbox extends React.Component {
data,
config: this.state.config,
onClickNode: this.onClickNode,
onDoubleClickNode: this.onDoubleClickNode,
onRightClickNode: this.onRightClickNode,
onClickGraph: this.onClickGraph,
onClickLink: this.onClickLink,
Expand Down
37 changes: 29 additions & 8 deletions src/components/graph/Graph.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ import utils from "../../utils";
* window.alert('Clicked node ${nodeId}');
* };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ignore this one 😄

*
* const onDoubleClickNode = function(nodeId) {
* window.alert('Double clicked node ${nodeId}');
* };
*
* const onRightClickNode = function(event, nodeId) {
* window.alert('Right clicked node ${nodeId}');
* };
Expand Down Expand Up @@ -92,6 +96,7 @@ import utils from "../../utils";
* config={myConfig}
* onClickGraph={onClickGraph}
* onClickNode={onClickNode}
* onDoubleClickNode={onDoubleClickNode}
* onRightClickNode={onRightClickNode}
* onClickLink={onClickLink}
* onRightClickLink={onRightClickLink}
Expand Down Expand Up @@ -223,15 +228,21 @@ export default class Graph extends React.Component {

/**
* Configures zoom upon graph with default or user provided values.<br/>
* NOTE: in order for users to be able to double click on nodes, we
* are disabling the native dblclick.zoom from d3 that performs a zoom
* whenever a user double clicks on top of the graph.
* {@link https://github.com/d3/d3-zoom#zoom}
* @returns {undefined}
*/
_zoomConfig = () =>
d3Select(`#${this.state.id}-${CONST.GRAPH_WRAPPER_ID}`).call(
d3Zoom()
.scaleExtent([this.state.config.minZoom, this.state.config.maxZoom])
.on("zoom", this._zoomed)
);
_zoomConfig = () => {
d3Select(`#${this.state.id}-${CONST.GRAPH_WRAPPER_ID}`)
.call(
d3Zoom()
.scaleExtent([this.state.config.minZoom, this.state.config.maxZoom])
.on("zoom", this._zoomed)
)
.on("dblclick.zoom", null);
};

/**
* Handler for 'zoom' event within zoom config.
Expand Down Expand Up @@ -267,7 +278,7 @@ export default class Graph extends React.Component {
};

/**
* Collapses the nodes, then calls the callback passed to the component.
* Collapses the nodes, then checks if the click is doubled and calls the callback passed to the component.
* @param {string} clickedNodeId - The id of the node where the click was performed.
* @returns {undefined}
*/
Expand All @@ -293,7 +304,15 @@ export default class Graph extends React.Component {
() => this.props.onClickNode && this.props.onClickNode(clickedNodeId)
);
} else {
this.props.onClickNode && this.props.onClickNode(clickedNodeId);
if (!this.nodeClickTimer) {
this.nodeClickTimer = setTimeout(() => {
this.props.onClickNode && this.props.onClickNode(clickedNodeId);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.nodeClickTimer = null;
}, CONST.TTL_DOUBLE_CLICK_IN_MS);
} else {
this.props.onDoubleClickNode && this.props.onDoubleClickNode(clickedNodeId);
this.nodeClickTimer = clearTimeout(this.nodeClickTimer);
}
}
};

Expand Down Expand Up @@ -469,13 +488,15 @@ export default class Graph extends React.Component {

componentWillUnmount() {
this.pauseSimulation();
this.nodeClickTimer && clearTimeout(this.nodeClickTimer);
}

render() {
const { nodes, links, defs } = graphRenderer.renderGraph(
this.state.nodes,
{
onClickNode: this.onClickNode,
onDoubleClickNode: this.onDoubleClickNode,
onRightClickNode: this.props.onRightClickNode,
onMouseOverNode: this.onMouseOverNode,
onMouseOut: this.onMouseOutNode,
Expand Down
1 change: 1 addition & 0 deletions src/components/graph/graph.const.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ export default {
},
LINK_CLASS_NAME: "link",
NODE_CLASS_NAME: "node",
TTL_DOUBLE_CLICK_IN_MS: 300,
...CONST,
};