Allow nodes to be dragged on to empty cells #59
Conversation
Made empty nodes mergeable
There was a problem hiding this comment.
It's very fun to play with it! However I found a few issues:
-
Dragging nodes onto loops resets loop properties
-
Node merges break undo/redo for some reason
-
After node merge, trying to create an arrow that starts on the merged node results in an uncaught exception
-
Creating an arrow on a blank canvas results in an uncaught exception
-
While dragging nodes around, it doesn't display label on a mergeable node:
| @@ -254,7 +254,8 @@ export default class App extends Component { | |||
| return this.moveInHistory(1) | |||
| } | |||
|
|
|||
| handleDataChange = evt => { | |||
| handleDataChange = (evt, overwriteLastHistory = false) => { | |||
There was a problem hiding this comment.
Why do we need overwriteLastHistory?
There was a problem hiding this comment.
Moving a node creates a entry in the history, and merging it would create another one. If I wouldn't overwrite the last history entry (the move) with the move and the merge, one could click undo and would have two not-merged nodes on the same position.
There was a problem hiding this comment.
I thought I fixed the undo/redo by doing so, but apparently I broke it again :/
There was a problem hiding this comment.
I added the true in the wrong spot. Weird that I didn't catch this before
src/components/Grid.js
Outdated
| if (existingNode.value.length > 0 && draggedNode.value.length > 0) | ||
| return | ||
| if (!arrEquals(newPosition, draggedNode.position)) | ||
| nodesToJoin = [draggedNode, existingNode] |
There was a problem hiding this comment.
I'd like nodesToJoin to be an object instead: {draggedNode, existingNode}
src/components/Grid.js
Outdated
| nodes: this.props.data.nodes | ||
| .filter(node => node.id != textNode.id) | ||
| .map(node => | ||
| node.id == emptyNode.id |
src/components/Grid.js
Outdated
| if (edge.from == textNode.id) edge.from = emptyNode.id | ||
| if (edge.to == textNode.id) edge.to = emptyNode.id | ||
| if (edge.to == edge.from) |
src/components/Grid.js
Outdated
| if (edge.from == textNode.id) edge.from = emptyNode.id | ||
| if (edge.to == textNode.id) edge.to = emptyNode.id | ||
| if (edge.to == edge.from) | ||
| edge = {...edge, loop: [0, false], labelPosition: 'right'} |
There was a problem hiding this comment.
Why are we resetting the loop properties?
There was a problem hiding this comment.
It wasn't meant to reset the loop properties, but rather set them the first time, when the start of an arrow is dragged to its head. I forgot that the properties might already be set.
yishn
changed the title
|
I think I fixed everything |
|
Hi, I really don't want to pressure you, but I also really would like this PR to count towards hacktoberfest, so if you could accept it before the end of the month, that would be great! 😄 |
Nodes can be merged now. Merging nodes A and B only works, if at least one node doesn't contain text (is empty).
Merging can be done by dragging:
Dragging the base of an arrow to the target of it, turns the arrow into a loop.
Everything seems to be working fine, but I'm not 100% sure I caught all bugs, so tell me if I introduced any bugs.