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

fix(space-tool): move and resize elements in correct order #364

Merged
merged 14 commits into from
Feb 6, 2020
Merged

fix(space-tool): move and resize elements in correct order #364

merged 14 commits into from
Feb 6, 2020

Conversation

philippfromme
Copy link
Contributor

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jun 13, 2019
@philippfromme philippfromme force-pushed the 1019-fix-space-tool branch 4 times, most recently from f06d68a to 045e55a Compare June 14, 2019 12:24
@nikku nikku added ready Ready to be worked on backlog Queued in backlog and removed in progress Currently worked on ready Ready to be worked on labels Jun 18, 2019 — with bpmn-io-tasks
@philippfromme philippfromme force-pushed the 1019-fix-space-tool branch 6 times, most recently from 81b981b to 15244c7 Compare January 15, 2020 13:43
@philippfromme philippfromme added needs review Review pending and removed backlog Queued in backlog labels Jan 15, 2020 — with bpmn-io-tasks
@philippfromme philippfromme changed the title WIP fix(space-tool): move and resize shapes in correct order fix(space-tool): move and resize shapes in correct order Jan 15, 2020
@philippfromme philippfromme changed the title fix(space-tool): move and resize shapes in correct order fix(space-tool): move and resize elements in correct order Jan 15, 2020
@nikku nikku changed the base branch from master to develop January 15, 2020 14:54
nikku
nikku reviewed Jan 15, 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.

I can confirm that this is an improvement to the current situation.

One issue I've found which may or may not be related is that we do not disable auto-resize during space tool. We should consider disabling that one (along with the other behaviors).

Otherwise, given certain diagrams funny things like this happen:

capture sg667S_optimized

lib/features/modeling/cmd/MoveShapeHandler.js Outdated Show resolved Hide resolved
@philippfromme
Copy link
Contributor Author

philippfromme commented Jan 16, 2020 •
edited
Loading

we do not disable auto-resize during space tool

This shouldn't happen. The autoResize: false hint is missing when resizing shapes. I'll fix that.

@philippfromme philippfromme changed the title fix(space-tool): move and resize elements in correct order WIP fix(space-tool): move and resize elements in correct order Jan 17, 2020
@philippfromme philippfromme force-pushed the 1019-fix-space-tool branch 3 times, most recently from fe3439e to c2effd0 Compare January 17, 2020 13:57
@nikku
Copy link
Member

nikku commented Jan 20, 2020

Issue: SpaceTool#calculateAdjustment has been removed.

It is used in bpmn-js, tough.

@nikku nikku self-requested a review January 20, 2020 12:43
nikku
nikku requested changes Jan 20, 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.

Need to restore bpmn-js compatibility.

@nikku nikku added ready Ready to be worked on and removed needs review Review pending labels Jan 24, 2020 — with bpmn-io-tasks
@nikku nikku self-requested a review February 6, 2020 09:12
philippfromme and others added 14 commits February 6, 2020 10:13
@philippfromme @nikku
fix(space-tool): add/remove space without side effects
4a02597
@philippfromme @nikku
chore(space-tool): refactor
66f96c6
@philippfromme @nikku
fix(grid-snapping): snap x/y when snapping dx/dy
7199e6b
@philippfromme @nikku
feat(space-tool): consider children when resizing
4d08d43
@philippfromme @nikku
feat(space-tool): consider parent when moving shape
a721b7f
@philippfromme @nikku
fix(space-tool): consider minimum size when resizing shape
a596563
@philippfromme @nikku
chore(space-tool): add tests
34b400e 
* refactor tests
* add tests
@philippfromme @nikku
chore(project): fix linting issues
9b2d72b
@nikku
test(grid-snapping): cover vertical space tool snapping
70a4dae
@nikku
test(space-tool): cover minimum height
2621a5d
@nikku
test(space-tool): cover min constraints resize from top and left
f71298c
@nikku
test(space-tool): verify waypoint origin movement
1c3e553
@philippfromme @nikku
fix(space-tool): provide labelBehavior=false hint on update waypoints
3f673cf 
* verify hints for move, resize and update waypoints
@philippfromme @nikku
chore(space-tool): rename SpaceTool#getElements > calculateAdjustments
aeb6670
@nikku
Copy link
Member

nikku commented Feb 6, 2020

There is one last case that is not properly taken care of (yet).

capture Eizn9J_optimized

Given there exist diagonal connections, the label does not move with the connection. This may break the label <> connection layout as shown in the screencapture above.

How big of a deal is it, though? Let's discuss 💬.

@philippfromme
Copy link
Contributor Author

How big of a deal is it, though? Let's discuss 💬.

This is exactly the behavior I'd expect. Better no magic at all than no magic plus one exception where we don't even know if that's helping the user. This can happen with any connection.

no-magic

@nikku
Copy link
Member

nikku commented Feb 6, 2020

O.K.

So let's not consider this a blocker then. Full 🐎 ahead 📯.

@nikku nikku merged commit aeb6670 into develop Feb 6, 2020
@bpmn-io-tasks bpmn-io-tasks bot removed the ready Ready to be worked on label Feb 6, 2020
@fake-join fake-join bot deleted the 1019-fix-space-tool branch February 6, 2020 12:47
@barmac barmac mentioned this pull request Mar 20, 2020
Closed
@philippfromme philippfromme mentioned this pull request Mar 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku Awaiting requested review from nikku

Assignees

@philippfromme philippfromme

Labels
None yet
Milestone
No milestone
Development

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