[FEAT] use fixed size Activity Marker #539
Conversation
aibcmars
added
enhancement
tbouffard
changed the title
aibcmars
force-pushed
the
465-activity_marker_fixed_size
branch
from
d6aeea4 to
8328f10
Compare
aibcmars
force-pushed
the
465-activity_marker_fixed_size
branch
from
8328f10 to
43de076
Compare
aibcmars
removed
the
depends on another PR ⚠️
There was a problem hiding this comment.
@aibcmars could you add
- screenshots of the change: we try do this systematically with @csouchet as it helps a lot for the review. You can use https://github.com/process-analytics/bpmn-visualization-examples/blob/4086f6f9eda8e1067061b59baacd3a2135318dd5/bpmn-files/all_activity_types.bpmn that shows
- add a new visualization non-regression tests for this use case (you can start from the bpmn file above). Currently, the test shows that the size has changed, but not that it doesn't depend on the activity size anymore
Marker size depends on the Activity size (screenshot from version 0.1.7)
|
|
There was a problem hiding this comment.
Still some minor things to change.
🎯 coverage of the targeted feature
This PR does not fully cover #465 although the size of the marker doesn't change with the activity size anymore.
We currently have no control over the size of the icon: we paint it with the size used in the IconPainter implementation
- so the loop doesn't have the same size than the other markers
- changing the size of marker in the
IconPainterimplementation or provide a custom implementation affect the rendering (see screenshot below) - it is not possible to adjust the marker size globally like we can do for marker margin, wich is going to be an issue when we will review the way we render all elements to have a consistent view across the BPMN diagram
However, as this PR already brings values to the lib (apparent marker fixed size, testing automation), I suggest that we can keep the implementation as it is and that we will cover the remaining points later.
Parallel multi-instantiation original icon size changed to 32x32
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <bpmn:definitions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" id="Definitions_1uz6fwe" targetNamespace="http://bpmn.io/schema/bpmn" exporter="bpmn-js (https://demo.bpmn.io)" exporterVersion="7.3.0"> | ||
| <bpmn:process id="Process_083f5uf" isExecutable="false"> | ||
| <bpmn:receiveTask id="instantiating_receive_task_id" instantiate="true"> |
There was a problem hiding this comment.
- we are only focussing on markers, receive task displayed an extra icon not related to what we want to test
- about maintenance, any receive task icon change would make the test fail whereas not related to a change about the marker. This would send a false signal about potential issue with the markers rendering
I would also add tasks with several marker to confirm that the whole marker group size is not impacted by the size of the activity
| canvas.moveTo(7.5, 14.08); | ||
| canvas.lineTo(5.75, 19.08); | ||
| canvas.lineTo(0, 17.08); |
There was a problem hiding this comment.
ℹ️ this seems to have an impact on the rendering that is not nice when zooming (for instance, zoom on the image provided in #539 (comment)).
Anyway, the loop marker may be reworked as part of #475
covers #465