Skip to content

Conversation

@yeexinc
Copy link
Contributor

@yeexinc yeexinc commented May 5, 2017

Purpose

Duplicate connectors when the user does a ctrl + click on input ports.
duplicatectrl

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@benglin
@riteshchandawar

@riteshchandawar
Copy link
Contributor

Functionality wise this is what I was looking, so LGTM from my side.

@benglin Can you please do the code review?

Thanks,
Ritesh

@riteshchandawar riteshchandawar requested a review from benglin May 5, 2017 07:00

case MakeConnectionCommand.Mode.EndAndStartCtrlConnection:
EndAndStartCtrlConnection(nodeId, command.PortIndex, command.Type);
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yeexinc, after further considerations I think better names for the modes would be BeginDuplicateConnection and BeginCreateConnections. Please also rename the methods to reflect this.

activeStartPorts = new PortModel[] { portModel.Connectors[0].Start };
firstStartPort = portModel.Connectors[0].Start;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Will the undo work for each of the connectors created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benglin - Yes, each undo action corresponds to one connector created.

@Racel
Copy link
Contributor

Racel commented Jul 18, 2017

@QilongTang - I think we just need to add this to our backlog. Let us just try to merge this in if you think it looks good to go. Adding a task for this.

@alfarok
Copy link
Contributor

alfarok commented Mar 13, 2019

closing and picking this work up in #9567

@alfarok alfarok closed this Mar 13, 2019
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