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

Jump to bottom

Connect Reverse #427

Merged
merged 8 commits into from
Nov 18, 2019
Merged

Connect Reverse #427

merged 8 commits into from
Nov 18, 2019

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Nov 4, 2019 •
edited by nikku
Loading

Closes #386

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Nov 4, 2019
@philippfromme philippfromme changed the title feat(modeling): connect reverse Connect Reverse Nov 4, 2019
This was referenced Nov 4, 2019
Merged
Merged
@nikku
Copy link
Member

nikku commented Nov 5, 2019

I've added a few comments, hoping that they are useful.

@philippfromme philippfromme force-pushed the 386-connect-reverse branch 4 times, most recently from b786acc to fdd29c3 Compare November 5, 2019 15:02
@philippfromme philippfromme marked this pull request as ready for review November 6, 2019 08:26
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Nov 6, 2019
@philippfromme philippfromme force-pushed the 386-connect-reverse branch 3 times, most recently from 6ef97c7 to c54a29a Compare November 6, 2019 17:36
@philippfromme philippfromme requested a review from nikku November 7, 2019 07:24
@nikku
Copy link
Member

nikku commented Nov 7, 2019

There is test coverage missing in some path that seem to be relevant to new features you've implemented. Could you have a look and verify if that is the case?

nikku
nikku requested changes Nov 7, 2019
View reviewed changes
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Please decide whether to remove reconnect{START|END} completely.

If we do that, clearly document it as part of the offending commit

BREAKING CHANGES:

* This happened, yo. Use strategy _X_ to migrate

Consider throwing descriptive errors if we remove API from modeling or make the legacy API work based on top of the new feature. If that is what we decide to do, we're going to keep around these bits forever maybe. Probably.

lib/features/bendpoints/BendpointMove.js Outdated Show resolved Hide resolved
lib/features/bendpoints/BendpointMove.js Outdated Show resolved Hide resolved
lib/features/bendpoints/BendpointMovePreview.js Outdated Show resolved Hide resolved
lib/features/bendpoints/BendpointMovePreview.js Outdated Show resolved Hide resolved
lib/features/bendpoints/BendpointSnapping.js Outdated Show resolved Hide resolved
lib/features/connect/Connect.js Show resolved Hide resolved
lib/features/modeling/Modeling.js Outdated Show resolved Hide resolved
lib/features/modeling/Modeling.js Show resolved Hide resolved
lib/features/modeling/cmd/ReconnectConnectionHandler.js Outdated Show resolved Hide resolved
test/spec/features/attach-support/AttachSupportSpec.js Show resolved Hide resolved
@philippfromme philippfromme force-pushed the 386-connect-reverse branch 2 times, most recently from 954e9d2 to cddefe5 Compare November 14, 2019 08:13
@philippfromme philippfromme requested a review from nikku November 14, 2019 08:32
@nikku nikku force-pushed the 386-connect-reverse branch from cddefe5 to e55d0eb Compare November 18, 2019 10:31
@philippfromme @nikku
feat(modeling): connect reverse
6110fc1 
Connect and reconnect allow connections to be previewed and modeled in
reverse order. To accomplish this rules will be checked for both (START -> END)
as well as (END -> START).

This feature comes with minor API reworks and must be supported downstream
by updated Layouter implementation.

* add Modeling#reconnect for reconnecting both source and target
* Layouter#layoutConnection receives waypoints hint that needs to be
  taken into account to preview reverse connections

BREAKING CHANGES:

* connection.reconnectStart and connection.reconnectEnd rules are replaced
  with connection.reconnect rule
* Connect context data changed { source, sourcePosition } -> { start, connectionStart }
@philippfromme @nikku
chore(bendpoints): add tests for bendpoint move preview
c63235b 
* ensure connection preview works reversely
@philippfromme @nikku
chore(bendpoints): use constant
665f877
@philippfromme @nikku
chore(bendpoints): remove duplicate code
b3d8161
@philippfromme @nikku
chore(bendpoints): fix comment
750faad
@philippfromme @nikku
chore(modeling): fix error message
b6203a1
@philippfromme @nikku
chore(modeling): remove <connection.reconnectStart/End> command
7454269 
BREAKING CHANGES:

* <connection.reconnectStart/End> commands removed
* Modeling#reconnectStart/End still available for convenience
@philippfromme @nikku
feat(bendpoints): do not reconnect if source and target not changed
b5b89bd
@nikku nikku force-pushed the 386-connect-reverse branch from e55d0eb to b5b89bd Compare November 18, 2019 10:35
nikku
nikku approved these changes Nov 18, 2019
View reviewed changes
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Looking good.

@fake-join fake-join bot merged commit 0ad1066 into develop Nov 18, 2019
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 18, 2019
@fake-join fake-join bot deleted the 386-connect-reverse branch November 18, 2019 10:38
@nikku
Copy link
Member

nikku commented Nov 18, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to swap source and target on connect and reconnect
2 participants
@philippfromme @nikku