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.

Sign up for GitHub

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

Jump to bottom

Fix #240 - cannot delete nodes with multiple graphs. #246

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Fix #240 - cannot delete nodes with multiple graphs. #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/edge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ class Edge extends React.Component<IEdgeProps> {
) {
let response = Edge.getDefaultIntersectResponse();

if (trg[nodeKey] == null) {
if (trg == null || trg[nodeKey] == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes a minor console error that shows up when a node is deleted and the connected edges try to calculate their positions.

return response;
}

Expand Down
52 changes: 33 additions & 19 deletions src/components/graph-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,7 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
const { initialBBox, zoomDelay, minZoom, maxZoom } = this.props;

if (this.viewWrapper.current) {
this.viewWrapper.current.addEventListener(
'keydown',
this.handleWrapperKeydown
);
document.addEventListener('keydown', this.handleWrapperKeydown);
this.viewWrapper.current.addEventListener(
'click',
this.handleDocumentClick
Expand Down Expand Up @@ -239,10 +236,7 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
}

componentWillUnmount() {
this.viewWrapper.current.removeEventListener(
'keydown',
this.handleWrapperKeydown
);
document.removeEventListener('keydown', this.handleWrapperKeydown);
this.viewWrapper.current.removeEventListener(
'click',
this.handleDocumentClick
Expand Down Expand Up @@ -400,10 +394,15 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
});

// remove node
// The timeout avoids a race condition
setTimeout(() => {
GraphUtils.removeElementFromDom(`node-${nodeId}-container`);
});
const timeoutId = `nodes-${nodeId}`;

// cancel an asyncRenderNode animation
cancelAnimationFrame(this.nodeTimeouts[timeoutId]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the race condition bug by cancelling the asyncRenderNode method. Now nodes are deleted and stay deleted.


GraphUtils.removeElementFromDom(
`node-${nodeId}-container`,
this.viewWrapper.current
);
}
}

Expand Down Expand Up @@ -463,7 +462,10 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
removeEdgeElement(source: string, target: string) {
const id = `${source}-${target}`;

GraphUtils.removeElementFromDom(`edge-${id}-container`);
GraphUtils.removeElementFromDom(
`edge-${id}-container`,
this.viewWrapper.current
);
}

canSwap(sourceNode: INode, hoveredNode: INode | null, swapEdge: any) {
Expand Down Expand Up @@ -492,7 +494,10 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
});

// remove from UI
GraphUtils.removeElementFromDom(`node-${nodeId}-container`);
GraphUtils.removeElementFromDom(
`node-${nodeId}-container`,
this.viewWrapper.current
);

// inform consumer
this.props.onSelectNode(null);
Expand Down Expand Up @@ -526,10 +531,12 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
if (selectedEdge.source != null && selectedEdge.target != null) {
// remove extra custom containers just in case.
GraphUtils.removeElementFromDom(
`edge-${selectedEdge.source}-${selectedEdge.target}-custom-container`
`edge-${selectedEdge.source}-${selectedEdge.target}-custom-container`,
this.viewWrapper.current
);
GraphUtils.removeElementFromDom(
`edge-${selectedEdge.source}-${selectedEdge.target}-container`
`edge-${selectedEdge.source}-${selectedEdge.target}-container`,
this.viewWrapper.current
);
}

Expand Down Expand Up @@ -605,7 +612,7 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
}
};

handleEdgeSelected = e => {
handleEdgeSelected = (e: any) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor flowtype redline fix.

const { source, target } = e.target.dataset;
let newState = {
svgClicked: true,
Expand Down Expand Up @@ -777,7 +784,10 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
return;
}

GraphUtils.removeElementFromDom('edge-custom-container');
GraphUtils.removeElementFromDom(
'edge-custom-container',
this.viewWrapper.current
);

if (edgeEndNode) {
const mapId1 = `${hoveredNodeData[nodeKey]}_${edgeEndNode[nodeKey]}`;
Expand Down Expand Up @@ -1085,7 +1095,10 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
const draggedEdgeCopy = { ...this.state.draggedEdge };

// remove custom edge
GraphUtils.removeElementFromDom('edge-custom-container');
GraphUtils.removeElementFromDom(
'edge-custom-container',
this.viewWrapper.current
);
this.setState(
{
draggedEdge: null,
Expand Down Expand Up @@ -1320,6 +1333,7 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
}

const containerId = `${id}-container`;

let nodeContainer:
| HTMLElement
| Element
Expand Down
55 changes: 48 additions & 7 deletions src/examples/multiple-graphs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,54 @@ import sample1 from './graph1-sample';
import sample2 from './graph2-sample';

type IGraphProps = {};
type IGraphState = {};
type IGraphState = {
selectedNode: any,
selectedNode2: any,
sample1: any,
sample2: any,
};

class Graph extends React.Component<IGraphProps, IGraphState> {
GraphViewRef;

constructor(props: IGraphProps) {
super(props);
this.GraphViewRef = React.createRef();
this.state = {
selectedNode: null,
selectedNode2: null,
sample1: sample1,
sample2: sample2,
};
}

onDeleteNode = (
viewNode: INode,
nodeId: string,
nodeArr: INode[],
configVar: any,
stateKey: string
) => {
// Delete any connected edges
const newEdges = configVar.edges.filter((edge, i) => {
return (
edge.source !== viewNode[NODE_KEY] && edge.target !== viewNode[NODE_KEY]
);
});

configVar.nodes = nodeArr;
configVar.edges = newEdges;
this.setState({
[stateKey]: configVar,
});
};

/*
* Render
*/

render() {
const { sample1, sample2 } = this.state;
const { nodes: graph1Nodes, edges: graph1Edges } = sample1;
const { nodes: graph2Nodes, edges: graph2Edges } = sample2;
const { NodeTypes, NodeSubtypes, EdgeTypes } = GraphConfig;
Expand All @@ -61,15 +94,19 @@ class Graph extends React.Component<IGraphProps, IGraphState> {
nodeKey={NODE_KEY}
nodes={graph1Nodes}
edges={graph1Edges}
selected={null}
selected={this.state.selectedNode}
nodeTypes={NodeTypes}
nodeSubtypes={NodeSubtypes}
edgeTypes={EdgeTypes}
readOnly={false}
onSelectNode={() => {}}
onSelectNode={node => {
this.setState({ selectedNode: node });
}}
onCreateNode={() => {}}
onUpdateNode={() => {}}
onDeleteNode={() => {}}
onDeleteNode={(selected, nodeId, nodeArr) => {
this.onDeleteNode(selected, nodeId, nodeArr, sample1, 'sample1');
}}
onSelectEdge={() => {}}
onCreateEdge={() => {}}
onSwapEdge={() => {}}
Expand All @@ -82,15 +119,19 @@ class Graph extends React.Component<IGraphProps, IGraphState> {
nodeKey={NODE_KEY}
nodes={graph2Nodes}
edges={graph2Edges}
selected={null}
selected={this.state.selectedNode2}
nodeTypes={NodeTypes}
nodeSubtypes={NodeSubtypes}
edgeTypes={EdgeTypes}
readOnly={false}
onSelectNode={() => {}}
onSelectNode={node => {
this.setState({ selectedNode2: node });
}}
onCreateNode={() => {}}
onUpdateNode={() => {}}
onDeleteNode={() => {}}
onDeleteNode={(selected, nodeId, nodeArr) => {
this.onDeleteNode(selected, nodeId, nodeArr, sample2, 'sample2');
}}
onSelectEdge={() => {}}
onCreateEdge={() => {}}
onSwapEdge={() => {}}
Expand Down
1 change: 1 addition & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export type IEdgeType = IEdge;
export { default as GraphUtils } from './utilities/graph-util';
export { default as Node } from './components/node';
export type INodeType = INode;
// eslint-disable-next-line prettier/prettier
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes an annoying problem with prettier trying to fix the line breaks when it doesn't need to.

export { default as BwdlTransformer } from './utilities/transformers/bwdl-transformer';
export { GV as GraphView };
export type LayoutEngineType = LayoutEngineConfigTypes;
Expand Down
4 changes: 2 additions & 2 deletions src/utilities/graph-util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ class GraphUtils {
}
}

static removeElementFromDom(id: string) {
const container = document.getElementById(id);
static removeElementFromDom(id: string, searchElement?: any = document) {
const container = searchElement.querySelector(`#${id}`);

if (container && container.parentNode) {
container.parentNode.removeChild(container);
Expand Down