Vertically Auto-Place Elements In Collapsed Subprocesses #2205
Conversation
|
Thanks for the PR. I will look into this later this week. |
barmac
force-pushed
the
auto-place-in-vertical-sub-process
branch
from
d3685d8 to
3b15986
Compare
| parent = getParent(element, 'bpmn:Participant'); | ||
| if (parent) { | ||
| return isHorizontal(parent); | ||
| } else if (isAny(element, types)) { | ||
| } else if (is(element, 'bpmn:Participant')) { |
There was a problem hiding this comment.
Does it mean we don't care about vertical/horizontal Lane anymore?
There was a problem hiding this comment.
Good catch. We still care.
| var process; | ||
| for (process = getBusinessObject(element); process; process = process.$parent) { | ||
| if (is(process, 'bpmn:Process')) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // The direction may be specified in another diagram. We ignore that there | ||
| // could be multiple diagrams with contradicting properties based on the | ||
| // assumption that such BPMN files are unusual. | ||
| var pool = elementRegistry.find(function(shape) { | ||
| var businessObject = getBusinessObject(shape); | ||
| return businessObject && businessObject.get('processRef') === process; | ||
| }); |
There was a problem hiding this comment.
Unfortunately, for some elements on a collaboration diagram process ends up being undefined, and this leads to unexpected outcomes.
Screen.Recording.2024-08-02.at.14.27.40.mov
| * | ||
| * @return {boolean} false for vertical pools, lanes and their children. true otherwise | ||
| */ | ||
| export function isDirectionHorizontal(element) { | ||
| export function isDirectionHorizontal(element, elementRegistry) { |
There was a problem hiding this comment.
Since we are requiring an additional argument now to handle the additional use case, this could mean we are introducing a potentially breaking change of the util. I'd rather have an escape hatch to return the default value (true) in case elementRegistry is not passed except for the cases that we can solve with the old code.
There was a problem hiding this comment.
I've made the argument optional.
|
Thanks for another contribution of yours. |
|
@barmac Thanks for your review. I think I've adressed your comments as far as possible. By the way, there seem to be building issues, but I don't think they're caused by the PR. |
barmac
force-pushed
the
auto-place-in-vertical-sub-process
branch
from
2778216 to
215e0e7
Compare
| @@ -37,19 +42,48 @@ export function getParent(element, anyType) { | |||
| * Determines if the local modeling direction is vertical or horizontal. | |||
| * | |||
| * @param {Element} element | |||
| * @param {ElementRegistry} elementRegistry | |||
There was a problem hiding this comment.
| * @param {ElementRegistry} elementRegistry | |
| * @param {ElementRegistry} [elementRegistry] - provide to support collapsed subprocess direction |
| * | ||
| * @return {boolean} false for vertical pools, lanes and their children. true otherwise | ||
| */ | ||
| export function isDirectionHorizontal(element) { | ||
| export function isDirectionHorizontal(element, elementRegistry = undefined) { |
There was a problem hiding this comment.
Optional argument defaults to undefined when not provided:
| export function isDirectionHorizontal(element, elementRegistry = undefined) { | |
| export function isDirectionHorizontal(element, elementRegistry) { |
| } | ||
| } | ||
|
|
||
| if (elementRegistry === undefined) { |
There was a problem hiding this comment.
Let's use idiomatic JS:
| if (elementRegistry === undefined) { | |
| if (!elementRegistry) { |
|
CI failures seem to be caused by some GH Actions outage 🤷 |
Thank you. I have adapted the changes. Also, the builds finally pass. |
|
Can you please rebase this? There are some conflicts to resolve according to GH. |
sombrek
force-pushed
the
auto-place-in-vertical-sub-process
branch
from
756f509 to
7b1b727
Compare
sombrek
force-pushed
the
auto-place-in-vertical-sub-process
branch
from
7b1b727 to
fa01871
Compare
|
All should be clear now. |
|
Thanks! |
Proposed Changes
Closes #2127
This already worked for expanded sub processes.
Visual demo