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

Allow nodes to be dragged on to empty cells #59

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Allow nodes to be dragged on to empty cells #59

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c4fafae
Made empty nodes mergeable
F1nnM Oct 19, 2020
06f86e7
Merge pull request #1 from F1nnM/master2
F1nnM Oct 19, 2020
25f5423
fixed loops and history
F1nnM Oct 19, 2020
d76a1af
Made id comparisions use ===
F1nnM Oct 21, 2020
bd4c02a
Fixed history, made nodesToJoin Object
F1nnM Oct 21, 2020
73c0765
Removed console.logs
F1nnM Oct 21, 2020
093dbf4
Mooaar fixes
F1nnM Oct 21, 2020
3ed167c
Removed console.log
F1nnM Oct 21, 2020
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
3 changes: 2 additions & 1 deletion src/components/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,8 @@ export default class App extends Component {
return this.moveInHistory(1)
}

handleDataChange = evt => {
handleDataChange = (evt, overwriteLastHistory = false) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need overwriteLastHistory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 thought I fixed the undo/redo by doing so, but apparently I broke it again :/

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 added the true in the wrong spot. Weird that I didn't catch this before

if (overwriteLastHistory) this.historyPointer--
let edgeAdded =
this.state.diagram.edges.length + 1 === evt.data.edges.length
let historyEntry = {diagram: evt.data, time: Date.now()}
Expand Down
57 changes: 50 additions & 7 deletions src/components/Grid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ export default class Grid extends Component {
width: 0,
height: 0,
phantomArrow: null,
cellTypesetSizes: {}
cellTypesetSizes: {},
nodesToJoin: null
}
}

Expand All @@ -24,7 +25,7 @@ export default class Grid extends Component {
document.addEventListener('mouseup', () => {
this.mouseDown = null

let {phantomArrow} = this.state
let {phantomArrow, nodesToJoin} = this.state

if (phantomArrow != null) {
if (!arrEquals(phantomArrow.from, phantomArrow.to)) {
Expand Down Expand Up @@ -66,11 +67,45 @@ export default class Grid extends Component {
]

let {onDataChange = () => {}} = this.props
onDataChange({data: {nodes: newNodes, edges: newEdges}})
onDataChange({data: {nodes: newNodes, edges: newEdges}}, true)
}

this.setState({phantomArrow: null})
}
if (nodesToJoin != null) {
let [draggedNode, existingNode] = nodesToJoin

if (draggedNode.id !== existingNode.id) {
let [textNode, emptyNode] =
draggedNode.value.length > 0
? [draggedNode, existingNode]
: [existingNode, draggedNode]

let {onDataChange = () => {}} = this.props
onDataChange({
data: {
nodes: this.props.data.nodes
.filter(node => node.id != textNode.id)
.map(node =>
node.id == emptyNode.id
Copy link
Owner

Choose a reason for hiding this comment

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

Use === for id comparisons

? {
...emptyNode,
position: existingNode.position,
value: textNode.value
}
: node
),
edges: this.props.data.edges.map(edge => {
if (edge.from == textNode.id) edge.from = emptyNode.id
if (edge.to == textNode.id) edge.to = emptyNode.id
if (edge.to == edge.from)
Copy link
Owner

Choose a reason for hiding this comment

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

Use === for id comparisons

edge = {...edge, loop: [0, false], labelPosition: 'right'}
Copy link
Owner

Choose a reason for hiding this comment

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

Why are we resetting the loop properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return edge
})
}
})
}
}
})

document.addEventListener('mousemove', evt => {
Expand All @@ -91,13 +126,21 @@ export default class Grid extends Component {
cameraPosition: arrSubtract(this.mouseDown.cameraPosition, movement)
})
} else if (this.mouseDown.mode === 'move') {
let {nodeIndex} = this.mouseDown
let {nodeIndex, node: draggedNode} = this.mouseDown
if (nodeIndex < 0) return

let existingNode = this.props.data.nodes.find(n =>
arrEquals(n.position, newPosition)
let existingNode = this.props.data.nodes.find(
n => arrEquals(n.position, newPosition) && draggedNode.id != n.id
)
if (existingNode != null) return

let nodesToJoin = null
if (existingNode != null) {
if (existingNode.value.length > 0 && draggedNode.value.length > 0)
return
if (!arrEquals(newPosition, draggedNode.position))
nodesToJoin = [draggedNode, existingNode]
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like nodesToJoin to be an object instead: {draggedNode, existingNode}

}
this.setState({nodesToJoin})

let {onDataChange = () => {}} = this.props

Expand Down