Skip to content

Duplicate connections using ctrl + click - #7714

Merged
riteshchandawar merged 3 commits into
DynamoDS:masterfrom
yeexinc:b-click
Apr 18, 2017
Merged

riteshchandawar merged 3 commits into
DynamoDS:masterfrom
yeexinc:b-click

Conversation

@yeexinc

@yeexinc yeexinc commented Mar 17, 2017 •

Copy link
Copy Markdown
Contributor

Purpose

This PR is to address DYN-622.
Multiple connections can be created by holding down the control key and clicking on the desired ports.
gif2

  • Multiple connections can be created only when the starting port (the first port clicked while starting the connection) is an output port, since input ports cannot accept multiple connections.
  • As the starting port is an output port, the subsequent ports selected (to complete the connections) must be input ports. This is because output port -> output port connections are not supported.
  • Ctrl + click can be cancelled by clicking on the canvas.

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

@riteshchandawar

FYIs

@Racel

@Racel

Racel commented Mar 17, 2017

Copy link
Copy Markdown

Very nice @yeexinc. One thing that maybe we should consider regarding this and the multi-ouput port reconnect functionality is being able to hit ESC to completely get out of the command. Let me know what you think. Can't wait to play with this.

@riteshchandawar

Copy link
Copy Markdown
Contributor

@Racel Hitting Esc in the current scenario is same as what we do with Single wire connection.

For Ctrl+Click hitting Esc will terminate the duplicating operation, I hope that is what expected here.

Do let us know if you have additional comments.

@Racel

Racel commented Mar 17, 2017

Copy link
Copy Markdown

@riteshchandawar - I believe that hitting ESC should terminate the duplicating operation. Will it disconnect the wires that were connected during the operation?

Regarding the multi-ouput port reconnect feature, I think ESC should place the wire back where it came from.

@yeexinc

yeexinc commented Mar 20, 2017

Copy link
Copy Markdown
Contributor Author

Hi @Racel , currently the ESC button only cancels the current connection (and does not place the wire back). This is the same behaviour as pressing ESC while doing single wire connection as mentioned by @riteshchandawar . It will not disconnect the other wires that were connected during the operation.

The user can obtain the previous connections by hitting undoes, though. Each undo will restore one connection. E.g. if the user makes 4 connections using ctrl + click, then 4 undoes will be needed to restore the previous state.

@Racel

Racel commented Mar 20, 2017

Copy link
Copy Markdown

@yeexinc - This behavior is fine for this duplicate connection feature. But, I plan on filing a separate task to adjust the behavior of the multi-output port reconnect.

@Racel

Racel commented Apr 12, 2017 •

Copy link
Copy Markdown

yeexinc - This looks good. When are you all planning on merging so we can try out?

FYI @wynged - thanks for your ideas on this. Take a look.

@yeexinc

yeexinc commented Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

@Racel - I think the PR needs to be reviewed. @riteshchandawar Would you mind taking a look?

@Racel
Racel requested a review from riteshchandawar April 17, 2017 18:09
@Racel Racel added the PTAL Please Take A Look 👀 label Apr 17, 2017
@Racel

Racel commented Apr 17, 2017

Copy link
Copy Markdown

@riteshchandawar - Please review so that we can test :)

@riteshchandawar

Copy link
Copy Markdown
Contributor

Looks good to me, I am merging this now.

@riteshchandawar
riteshchandawar merged commit 650c5f5 into DynamoDS:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PTAL Please Take A Look 👀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants