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

expose "onMoveNode" method (optional), added "canDragNode" method (optional), center svg icons in control buttons #280

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

Conversation

man-person
Copy link

@man-person man-person commented Oct 11, 2020

needed to do some stuff while dragging a node so i added these.

may be relevant to issues: #281 #261 #195

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2020

CLA assistant check
All committers have signed the CLA.

added "canDragNode" method (optional)
@man-person man-person changed the title center svg icon in re-center control button expose "onMoveNode" method (optional), added "canDragNode" method (optional), center svg icons in control buttons Oct 22, 2020
@man-person
Copy link
Author

@9renpoto @ksnyder9801 I’d be grateful if you could review this one

@@ -77,6 +78,7 @@ export type IGraphViewProps = {
onSwapEdge?: (sourceNode: INode, targetNode: INode, edge: IEdge) => void,
onUndo?: () => void,
onUpdateNode?: (node: INode) => void,
Copy link
Contributor

@ajbogh ajbogh Nov 16, 2020

Choose a reason for hiding this comment

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

onUpdateNode should already tell you that the node moved. It passes the new X,Y coordinates within the INode type. Is it possible to call onUpdateNode instead of creating a new onMoveNode method?

Copy link

Choose a reason for hiding this comment

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

I think the problem with this is that onUpdateNode is only called when the move is finished, not while the move is in progress. That is also why one can not just return false from onUpdateNode and thereby prevent movement.

@@ -61,6 +61,7 @@ export type IGraphViewProps = {
hoveredNode: INode | null,
swapEdge: IEdge
) => boolean,
canDragNode?: (nodeId: string) => boolean,
Copy link
Contributor

@ajbogh ajbogh Nov 16, 2020

Choose a reason for hiding this comment

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

Change to canMoveNode or canUpdateNode?

@@ -72,6 +72,7 @@ class GraphView extends React.Component<IGraphViewProps, IGraphViewState> {
canSwapEdge: () => true,
canDeleteEdge: () => true,
canDeleteNode: () => true,
canDragNode: () => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to canMoveNode

if (!shiftKey && !draggingEdge) {
if (onMoveNode) {
onMoveNode(node);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This if statement needs to move within the if statement on line 809.


> svg {
vertical-align: sub;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rebase on master. This may be fixed.

Copy link
Contributor

@ajbogh ajbogh left a comment

Choose a reason for hiding this comment

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

Please rebase on master and then comment to justify your reasoning and update the PR based on the conversation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants