-
Notifications
You must be signed in to change notification settings - Fork 34
[FEAT] Detect Complex Gateway #2250
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
Conversation
♻️ PR Preview 80207b9 has been successfully destroyed since this PR has been closed. 🤖 By surge-preview |
♻️ PR Preview 80207b9 has been successfully destroyed since this PR has been closed. 🤖 By surge-preview |
@@ -1685,6 +1686,12 @@ | |||
<dc:Bounds x="2525" y="1250" width="90" height="14" /> | |||
</bpmndi:BPMNLabel> | |||
</bpmndi:BPMNShape> | |||
<bpmndi:BPMNShape id="shape_complex_gateway_id" bpmnElement="complex_gateway_id"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ personal note: check if the rendering is ok when displaying the diagram, otherwise change the coordinates 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I wasn't sure that we document it, but yes it is documented!
Please, adjust shape coordinates to ensure new elements can be easily shown with a viewer and don't overlap with existing elements
docs/contributors/testing.md
Outdated
* You have added changes in overlays: i.e. positioning, shapes, styling. | ||
<br/><br/> | ||
`overlays.rendering.test.ts` drives all visual tests for that part. A visual test only requires a BPMN diagram as input. | ||
- You aim supporting a new BPMN elements or when the rendering of the BPMN elements change (existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However all IDE don't format/lint markdown in the same way. So I will revert extra changes.
We have to find common lint rules that we can always apply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 7cc7846
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR which is globally good. Thanks for the documentation improvements based on your experience.
However, there are some elements that need to be changed
- The PR title doesn't follow what we are usually doing
- The PR description lacks of explanation and the link to the issue won't close it on PR merge (see https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests)
- The BPMN Support paragraph in the user document has not been updated (https://github.com/process-analytics/bpmn-visualization-js/blob/v0.26.2/docs/contributors/bpmn-support-how-to.md#elements-detection). If you need more guidance, please let me know. We can also improve the documentation about it. There is no need to display the new gateway icon for now. We will put it when the final rendering is implemented
- The PR used as an example to illustrate what to do for "BPMN element detection" are too old. We should mention this new PR and a more recent one instead
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Welcome aboard!
fixes #793
Screenshot