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

Enable Top-Down Modeling #453

Merged
merged 5 commits into from
Apr 7, 2020
Merged

Enable Top-Down Modeling #453

merged 5 commits into from
Apr 7, 2020

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Apr 1, 2020 •
edited
Loading

BREAKING CHANGES

  • rename deconflictPosition to getFreePosition and change API
  • change getConnectedDIstance API

Required by bpmn-io/dmn-js#492

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Apr 1, 2020
@philippfromme philippfromme changed the title 479 connection layout Enable Top-Down Modeling Apr 1, 2020
@philippfromme philippfromme mentioned this pull request Apr 1, 2020
Merged
@philippfromme philippfromme marked this pull request as ready for review April 1, 2020 14:44
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 1, 2020
@philippfromme philippfromme force-pushed the 479-connection-layout branch 2 times, most recently from 9caf58b to f43f92d Compare April 2, 2020 09:25
@philippfromme philippfromme requested review from nikku and barmac and removed request for nikku April 2, 2020 12:51
@nikku
Copy link
Member

nikku commented Apr 2, 2020

🆘

image

@barmac
Copy link
Member

barmac commented Apr 2, 2020

I'd love to see this part tested as I think nested ifs might be error-prone.

nikku
nikku reviewed Apr 2, 2020
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.

Some questions / comments inlined.

lib/features/bendpoints/BendpointMovePreview.js Outdated Show resolved Hide resolved
test/spec/features/auto-place/AutoPlaceSpec.js
@@ -176,67 +177,169 @@ describe('features/auto-place', function() {

describe('util', function() {

describe('#deconflictPosition', function() {
describe('#findFreePosition', function() {
Copy link
Member

Choose a reason for hiding this comment

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

From bpmn-js perspective this is a breaking change. Did you consider implement it in a non breaking fashion?

Alternatively, what is the proposed rollout strategy?

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 is. The result would be a major release of diagram-js. Alternatively, we could decide to not change the name.

lib/features/modeling/cmd/ReconnectConnectionHandler.js Show resolved Hide resolved
lib/draw/BaseRenderer.js Outdated Show resolved Hide resolved
lib/layout/ConnectionDocking.js Outdated Show resolved Hide resolved
lib/features/auto-place/AutoPlaceUtil.js Show resolved Hide resolved
test/spec/features/auto-place/AutoPlaceSpec.js Show resolved Hide resolved
@philippfromme
Copy link
Contributor Author

🆘

From my perspective the code coverage is okay.

@philippfromme philippfromme force-pushed the 479-connection-layout branch 3 times, most recently from 68c2ed6 to a7a7b9e Compare April 3, 2020 11:00
@philippfromme
Copy link
Contributor Author

I'd love to see this part tested as I think nested ifs might be error-prone.

Done.

@philippfromme philippfromme requested a review from nikku April 6, 2020 13:35
@barmac
Copy link
Member

barmac commented Apr 7, 2020

@philippfromme There is a merge conflict, have you noticed?

@philippfromme
Copy link
Contributor Author

philippfromme commented Apr 7, 2020 •
edited
Loading

@barmac Resolved. Resulted from a PR that was merged minutes ago.

@philippfromme philippfromme force-pushed the 479-connection-layout branch 3 times, most recently from 32322e9 to c4dcd76 Compare April 7, 2020 12:05
@fake-join fake-join bot merged commit 9d325eb into develop Apr 7, 2020
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Apr 7, 2020
@fake-join fake-join bot deleted the 479-connection-layout branch April 7, 2020 13:34
nikku
nikku reviewed Apr 7, 2020
View reviewed changes
lib/features/bendpoints/BendpointMove.js
insert: insert,
bendpointIndex: bendpointIndex
}
hints.bendpointMove = {
Copy link
Member

Choose a reason for hiding this comment

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

What additional info is being passed secretly down here? Does it make sense to be explicit and not monkey patch?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it does not matter, we monkey patch here, too: https://github.com/bpmn-io/diagram-js/pull/453/files#diff-f0a545ec36c40669962054112f362a58R170

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

@nikku nikku nikku left review comments

@barmac barmac barmac approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philippfromme @nikku @barmac