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

Add onClick handler to the canvas, for use in eg. unselecting nodes #113

Merged
merged 7 commits into from
Oct 2, 2018

Conversation

MathiasKoch
Copy link
Contributor

Add onClick handler to the canvas, for use in eg. unselecting nodes

Copy link
Owner

@danielcaldas danielcaldas left a comment

Choose a reason for hiding this comment

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

Please go through the comments that I left and also update this PR with the master of danielcaldas/react-d3-graph

@@ -50,6 +50,8 @@ export default class Sandbox extends React.Component {
};
}

onClickCanvas = () => console.info(`Clicked the canvas`);
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename this to onClickGraph, people may mistakenly think that under the hood canvas is being used when the full react-d3-graph implementation is based on SVG only.

@@ -0,0 +1,44 @@
import React from 'react';
Copy link
Owner

Choose a reason for hiding this comment

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

This should not be here you probably need to get your fork up to date with master

@@ -400,7 +414,7 @@ export default class Graph extends React.Component {

return (
<div id={`${this.state.id}-${CONST.GRAPH_WRAPPER_ID}`}>
<svg style={svgStyle}>
<svg name="canvas" style={svgStyle} onClick={this.onClickCanvas}>
Copy link
Owner

Choose a reason for hiding this comment

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

Again regarding the canvas reference, please rename it to svg-container-${this.state.id}. Why appending the this.state.id? To avoid collisions when there are more than one graph present in the same page (yes, this is a real use case)

@@ -113,6 +113,7 @@
* - 'wye'
*
* **[note]** react-d3-graph will map this values to [d3 symbols](https://github.com/d3/d3-shape#symbols)
* @param {Function} [node.viewGenerator=undefined] - 🔍🔍🔍 function that receives a node and returns a JSX view.
* <br/>
* @param {Object} link link object is explained in the next section. ⬇️
Copy link
Owner

Choose a reason for hiding this comment

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

Please add documentation on the new function that is being exposed in the react-d3-graph component API.

Copy link
Owner

@danielcaldas danielcaldas left a comment

Choose a reason for hiding this comment

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

Please discard the sandbox/rd3g.sandbox.bundle* changes

Copy link
Owner

@danielcaldas danielcaldas left a comment

Choose a reason for hiding this comment

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

Looking good now! Just commented on a small detail regarding the jsdoc of onClickGraph

@@ -372,6 +372,23 @@ export default class Graph extends React.Component {
this.props.onClickNode && this.props.onClickNode(clickedNodeId);
};

/**
* Calls the callback passed to the component.
* @param {string} e - The event of onClick handler.
Copy link
Owner

Choose a reason for hiding this comment

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

@param {Object} e is an Object not a string

* @param {Óbject} e - The event of onClick handler.
* @returns {undefined}
*/
onClickGraph = e => {
Copy link
Owner

Choose a reason for hiding this comment

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

Please add this method to the component documentation, just follow the example of one of the functions there like onClickNode

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 just added it to Readme docs. Is there anywhere else you would like me to add it?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, please add it as well to the jsdoc that is on the component in the file Graph.jsx

@danielcaldas danielcaldas added the Hacktoberfest Hacktoberfest label label Oct 2, 2018
@danielcaldas danielcaldas merged commit c13d136 into danielcaldas:master Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Hacktoberfest label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants