refactor: remove StyleUtils #2550
Conversation
Replace `StyleUtils` methods by direct calls: the methods were used only once, so this was an extra indirection (the inline code is explicit enough). Extract only the `getBpmnIsInstantiating` function because it is used twice and make it internal. It also changes the way we store `bpmn.isInitiating` in the mxGraph style. Previously, we stored the value from the BPMN Semantic as a string which required a subsequent parsing to know whether it was an "instantiate" value. This was also not consistent with the property name which suggests to store a boolean value. We now store a boolean value which can be directly used in the Shapes implementation. -------------------------------------------------------------------------------- BREAKING CHANGES: `StyleUtils` may have been used in rare cases to redefine the way the shapes are rendered. It wasn't used in the examples of `bpmn-visualization`.
|
♻️ PR Preview 5022bdf has been successfully destroyed since this PR has been closed.
🤖 By surge-preview |
|
🎊 PR Preview 5022bdf has been successfully built and deployed to https://process-analytics-bpmn-visualization-js-doc_preview-pr-2550.surge.sh 🕐 Build time: 0.022s
🤖 By surge-preview |
… stored. This fix test isses.
| @@ -158,7 +158,7 @@ export default class StyleComputer { | |||
| } | |||
|
|
|||
| computeMessageFlowIconStyle(edge: Edge): string { | |||
| return `shape=${BpmnStyleIdentifier.MESSAGE_FLOW_ICON};${BpmnStyleIdentifier.IS_INITIATING}=${edge.messageVisibleKind}`; | |||
| return `shape=${BpmnStyleIdentifier.MESSAGE_FLOW_ICON};${BpmnStyleIdentifier.IS_INITIATING}=${edge.messageVisibleKind === MessageVisibleKind.INITIATING}`; | |||
There was a problem hiding this comment.
issue (non-blocking):
In the BPMN specification, we can have 3 states for the messageVisibleKind: unspecified, initiating and non-initiating.
So we shouldn't add BpmnStyleIdentifier.IS_INITIATING to the style if messageVisibleKind is unspecified.
There was a problem hiding this comment.
computeMessageFlowIconStyle is only called when we detect that a message flow icon must be drawn, so the check is not done here but in insertMessageFlowIconIfNeeded.
bpmn-visualization-js/src/component/mxgraph/BpmnRenderer.ts
Lines 111 to 113 in 94ce4be
With the old and new implementation, once the bpmn.isInitiating style is present, an icon is displayed (filled or not). Here, we only change how the information is stored, not when it is stored.
All tests including e2e visual an integration tests pass. They may miss some use cases related to the BPMN specification.
In that case, they must be added (both integration and visual tests) but only because, at present, we may not support the unspecified messageVisibleKind.
There was a problem hiding this comment.
As far as I understand, we cover everything in the tests and in the implementation.
A MessageFlow always have the messageVisibleKind property set. NONE is set when it was undefined in the BPMN diagram.
There was a problem hiding this comment.
I though when we compute the classes that we always added messageVisibleKind 😄
It's ok for me like this 👍
# Conflicts: # test/unit/component/mxgraph/renderer/StyleComputer.test.ts
|
Kudos, SonarCloud Quality Gate passed! |
Replace
StyleUtilsmethods by direct calls: the methods were used only once, so this was an extra indirection (the inline code is explicit enough).Extract only the
getBpmnIsInstantiatingfunction because it is used twice and make it internal.It also changes the way we store
bpmn.isInitiatingin the mxGraph style. Previously, we stored the value from the BPMN Semantic as a string which required a subsequent parsing to know whether it was an "instantiate" value. This was also not consistent with the property name which suggests to store a boolean value.We now store a boolean value which can be directly used in the Shapes implementation.
BREAKING CHANGES:
StyleUtilsmay have been used in rare cases to redefine the way the shapes are rendered. It wasn't used in the examples ofbpmn-visualization.StyleUtilswas mark as experimental and as subject to change as part of BPMN Theme refactoring. So users already knew that a deletion could take place.Notes
This has been detected during the
[email protected]migration POC: #2366