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.

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

Prevent expanded subprocess overlap of previous content #1026

Conversation

gustavjf
Copy link
Contributor

@gustavjf gustavjf commented May 13, 2019

Which issue does this PR address?

Relates to camunda/camunda-modeler#1244

Description

This covers two scenarios:

  1. When a shape is replaced with an expanded subprocess

camunda-modeler#1244_01

  1. When a subprocess is toggled from collapsed to expanded

camunda-modeler#1244_02

Only when:

  1. There are incoming sequence flows (previous content)
  2. There are no outgoing sequence flows (following content)

Notes for reviewer

  1. The connection layout breaks when replacing/toggling, as shown in the screen captures above. This is a known issue.

  2. The CI fails on codecoverage due to missing tests for this and this code path.

Since pools do not trigger toggleCollapse events and we may decide to add support for other elements which can be collapsed in the future (i.e., collapsed sub-choreographies), I think it makes sense to ignore codecov and keep those conditions in the code. As far as I'm aware, we don't have other elements which can be collapsed via toggleCollapse except for subprocesses so trying to write a test for this may not make sense.

@gustavjf gustavjf self-assigned this May 13, 2019
@ghost ghost added the needs review Review pending label May 13, 2019
@gustavjf gustavjf force-pushed the adjust-expanded-subprocess-position-if-overlaps-previous-content branch 4 times, most recently from 2216a31 to 1597f18 Compare May 13, 2019 14:04
@gustavjf gustavjf changed the title WIP Prevent expanded subprocess overlap of previous content Prevent expanded subprocess overlap of previous content May 13, 2019
@gustavjf gustavjf added the enhancement New feature or request label May 13, 2019
This covers two scenarios:

1. When a shape is replaced with an expanded subprocess
2. When a subprocess is toggled from collapsed to expanded

Only when:

1. There are incoming sequence flows (previous content)
2. There are no outgoing sequence flows (following content)
@philippfromme philippfromme force-pushed the adjust-expanded-subprocess-position-if-overlaps-previous-content branch from 1597f18 to 7d05ff1 Compare May 15, 2019 14:30
@ghost ghost assigned philippfromme May 15, 2019
Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

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

I've changed a few things:

  • adjusted coding style
  • removed util in favor of helper functions
  • simplified toggle implementation
  • made sure collapsing sub process doesn't move it

⚡️

@philippfromme philippfromme merged commit 05fea05 into master May 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the adjust-expanded-subprocess-position-if-overlaps-previous-content branch May 15, 2019 14:57
@ghost ghost removed the needs review Review pending label May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants