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

Fixing undefined entity on node remove #34

Merged

Conversation

Lindsay89
Copy link
Contributor

Description

The fix consists in removing the links associated with a node, when this one deleted.

Related Issue

#33

@Lindsay89 Lindsay89 force-pushed the hotfix/undefined-entity-on-remove branch 7 times, most recently from 1c13650 to ec047df Compare October 22, 2020 07:04
@antonioru antonioru self-requested a review October 22, 2020 09:33
@antonioru antonioru added the bug Something isn't working label Oct 22, 2020
Copy link
Owner

@antonioru antonioru left a comment

Choose a reason for hiding this comment

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

Nice job, please check my final review

docs/dynamic-nodes.md Outdated Show resolved Hide resolved
docs/dynamic-nodes.md Outdated Show resolved Hide resolved
const [nodeId, setNodeId] = useState(schema.nodes.length+1);
const deleteNodeFromSchema = (id) => {
const nodeToRemove = schema.nodes.find(node => node.id === id);
removeNode(nodeToRemove);
Copy link
Owner

Choose a reason for hiding this comment

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

fix this indentation

docs/dynamic-nodes.md Show resolved Hide resolved
@@ -34,6 +34,13 @@ const Diagram = (props) => {
nodeRefs[nodeId] = nodeEl;
};

// when a node is deleted, remove its references
const onNodeRemove = (nodeId, inputsPorts, outputsPorts) => {
Copy link
Owner

Choose a reason for hiding this comment

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

perhaps we can use useCalback?

@@ -19,16 +20,23 @@ const schemaReducer = (state, action) => {
nodes: state.nodes || [],
links: state.links || [],
});
case ON_NODE_REMOVE:
case ON_NODE_REMOVE: { // remove all node's links
let linksToKeep = [];
Copy link
Owner

Choose a reason for hiding this comment

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

let nextLinks = state.links || [];

src/hooks/useSchema/schemaReducer.js Show resolved Hide resolved
src/shared/functions/createSchema.js Show resolved Hide resolved
@@ -5,6 +5,7 @@ import DiagramNode from '../dist/Diagram/DiagramNode/DiagramNode';

describe('DiagramNode component', () => {
afterEach(cleanup);
sinon.restore();
Copy link
Owner

Choose a reason for hiding this comment

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

afterEach(() => {
clearnup();
sinon.restore();
});

tests/getNodePortsId.spec.js Show resolved Hide resolved
@Lindsay89 Lindsay89 force-pushed the hotfix/undefined-entity-on-remove branch 3 times, most recently from b3aa847 to 772270b Compare October 22, 2020 14:25
@Lindsay89 Lindsay89 force-pushed the hotfix/undefined-entity-on-remove branch from 772270b to b5f85c8 Compare October 22, 2020 16:28
@antonioru antonioru merged commit d8af5cc into antonioru:master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants