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

[FEAT] Render all Cancel Events #789

Merged

Conversation

VickyPicky
Copy link
Contributor

[FEAT] Render cancel events

closes #654
image
image

@csouchet csouchet added BPMN rendering Something about the way the lib is rendering BPMN elements enhancement New feature or request external contribution 👤 Pull requests provided by someone who is not a core maintainer labels Oct 20, 2020
Copy link
Member

@csouchet csouchet left a comment

Choose a reason for hiding this comment

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

Litlle suggestions to simply the code, and 1 to align the cross correctly 😄

private static drawCrossIcon(canvas: BpmnCanvas, thickness: number, roundedEdge: boolean): void {
canvas.begin();
canvas.moveTo(0.5 - thickness, 0);
roundedEdge ? canvas.arcTo(0.5, 0.5, 0, 0, 1, 0.5 + thickness, 0) : canvas.lineTo(0.5 + thickness, 0);
Copy link
Member

Choose a reason for hiding this comment

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

As we can see in the BPMN specification, the cross for the Cancel event must not to be rounded:
image

So, to simplify, I suggest to keep just canvas.lineTo(....) 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a comment in issue #654 with an examples with non-rounded endings, please, check it :)

Copy link
Member

Choose a reason for hiding this comment

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

We answered on the issue with @aibcmars :)

/**
* This icon is used by `cancel events`.
*/
public paintCancelCrossIcon({ c, ratioFromParent, setIconOrigin, shape, icon }: PaintParameter): void {
Copy link
Member

Choose a reason for hiding this comment

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

As it's almost the same code as paintXCrossIcon(...), I suggest to keep only paintXCrossIcon(...) and pass the thickness in the icon parameter.
We need also to modify the method calling in gateway-shapes.ts to give isFilled: true in the icon parameter.

(paintParameter: PaintParameter) =>
this.iconPainter.paintCancelCrossIcon({
...paintParameter,
setIconOrigin: (canvas: BpmnCanvas) => canvas.setIconOriginToShapeTopLeft(2, 8),
Copy link
Member

Choose a reason for hiding this comment

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

There is a little space on the right (and not on the left).
image

Copy link
Member

Choose a reason for hiding this comment

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

I think it's more aligned with this:

Suggested change
setIconOrigin: (canvas: BpmnCanvas) => canvas.setIconOriginToShapeTopLeft(2, 8),
setIconOrigin: (canvas: BpmnCanvas) => canvas.setIconOriginToShapeTopLeft(2, 9),

Result
image

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a little space on the right (and not on the left).
image

there is something wrong as it renders well with file: model-complete-semantic.bpmn and badly with events.bpmn

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the size of the shape of the event are not the the same in the 2 files.
If it's the case, there is a problem with the positionning of the icon. We need to position it according to the size of the shape, not with a margin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@csouchet csouchet added the rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged label Oct 21, 2020
@aibcmars aibcmars force-pushed the 654-render_cancel_events branch from 7d7ca1d to 26c4593 Compare October 21, 2020 15:11
@aibcmars aibcmars removed the rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged label Oct 21, 2020

IconPainter.drawCrossIcon(canvas);
const rotationCenterX = shape.w / 4;
Copy link
Member

Choose a reason for hiding this comment

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

If someone change the ratioFromParent (the size of the icon from the size of the shape), we always want to rotate the icon with the same value.

Can you revert this change, please ?

Copy link
Member

Choose a reason for hiding this comment

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

Forget my previous comment.
I'm not sur to understand how it's works, but everthing is OK.

Thank you very much for having integrate my comments 😄

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's the calculation of coordinates of rotation center.
Previously it was hardcoded to 1/4th cause of initial size of icon (half-size of shape) => center of rotation was on 1/4th of shape, but must be proportional to the ratioFromParent.
BTW, the default ratioFromParent is equals to 0.25 ;)

this.iconPainter.paintXCrossIcon({ ...paintParameter, setIconOrigin: (canvas: BpmnCanvas) => canvas.setIconOriginToShapeTopLeftProportionally(4) });
this.iconPainter.paintXCrossIcon({
...paintParameter,
icon: { ...paintParameter.icon, isFilled: true },
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@csouchet csouchet left a comment

Choose a reason for hiding this comment

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

image
image
image

@csouchet csouchet added the hacktoberfest-accepted Accepted Pull Request during Hacktoberfest label Oct 21, 2020
@csouchet csouchet merged commit 0b81939 into process-analytics:master Oct 21, 2020
@csouchet csouchet changed the title [FEAT] Render cancel events [FEAT] Render all Cancel Events Oct 22, 2020
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 enhancement New feature or request external contribution 👤 Pull requests provided by someone who is not a core maintainer hacktoberfest-accepted Accepted Pull Request during Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] Render Cancel Events
3 participants