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

[FIX] Don't display elements of a Collapsed Sub-Process #772

Merged
merged 10 commits into from
Oct 16, 2020

Conversation

csouchet
Copy link
Member

Closes #741

Bug

image

Fix

image

@csouchet csouchet added the bug Something isn't working label Oct 14, 2020
@csouchet csouchet marked this pull request as ready for review October 14, 2020 17:08
@csouchet
Copy link
Member Author

@tbouffard @aibcmars Is it possible for you to generate the snapshot, because if it's generated from MacOs, there is a diffrence of 0.08862368927704534% for windows & 0.1126230944805684% for Unbuntu.

@tbouffard
Copy link
Member

tbouffard commented Oct 14, 2020

@tbouffard @aibcmars Is it possible for you to generate the snapshot, because if it's generated from MacOs, there is a diffrence of 0.08862368927704534% for windows & 0.1126230944805684% for Unbuntu.

done with a54c7b6
I removed the label as this is not something under test here, so no need for threshold!

@tbouffard tbouffard added the BPMN rendering Something about the way the lib is rendering BPMN elements label Oct 14, 2020
@tbouffard
Copy link
Member

tbouffard commented Oct 15, 2020

@csouchet I have proposed a refactoring to try to clean the Renderer code (2 commits)

  • the 1st one extract the conversion code to better split responsibilities
  • the 2nd one (0fa93bb) was an attempt to avoid duplication in filter predicates and avoid multi flownodes traversal. I am not fan of the if/else I introduced, so IMHO, we should revert this one. I kept it to know what you think about it and if you see some possible improvements.

@csouchet csouchet force-pushed the 741-Don't_display_elements_of_a_Collapsed_Sub-Process branch from 0fa93bb to 9fc2d92 Compare October 15, 2020 08:08
}),
);
this.insertEdges(bpmnModel.edges);
this.insertShapes(displayedModel.pools);
Copy link
Member Author

Choose a reason for hiding this comment

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

👍 It's easier to read, but instead of adding a new class, I will have rather modify the BPMNModel, since we already distinguish between Lane / Pool / Flownode in the shapes.

Copy link
Member

Choose a reason for hiding this comment

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

Why not but this means removing from the model children of the collapsed subprocess.
I don't see any impacts for now for the rendering but I haven't think about future features that could be impacted by such a change.

Copy link
Member

@tbouffard tbouffard Oct 15, 2020

Choose a reason for hiding this comment

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

Can we do such a refactoring in another PR. With the current changes, everything occur only in the Render file?
Changes directly in the model will have more impacts, so to keep this PR foccused on the fix, this is probably better to limit changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem 😄

const subprocesses: Shape[] = [];
const boundaryEvents: Shape[] = [];
const otherFlowNodes: Shape[] = [];
bpmnModel.flowNodes.forEach(shape => {
Copy link
Member Author

Choose a reason for hiding this comment

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

👍

ℹ️ If we change BPMNModel, no need this

Copy link
Member

Choose a reason for hiding this comment

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

So what do we do for this code. Do we revert the last commit to restore the 3 array filters or do we keep it like this?
cc @aibcmars

Copy link
Member Author

Choose a reason for hiding this comment

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

it's ok for me :)

@csouchet csouchet merged commit 873c1b0 into master Oct 16, 2020
@csouchet csouchet deleted the 741-Don't_display_elements_of_a_Collapsed_Sub-Process branch October 16, 2020 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BPMN rendering Something about the way the lib is rendering BPMN elements bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Don't display elements of a Collapsed Sub-Process
2 participants