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 Text Annotation #405

Merged
merged 8 commits into from
Jul 8, 2020
Merged

Conversation

aibcmars
Copy link
Contributor

@aibcmars aibcmars commented Jul 8, 2020

closes #386

@aibcmars aibcmars added enhancement New feature or request BPMN rendering Something about the way the lib is rendering BPMN elements labels Jul 8, 2020
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

I am ok with the way of managing the shape but the rendering can by improved IMHO

  • label positioning: I proposed bc7c30f as a starting point. Label margin/spacing should be improved
  • shape border position and size + configurability
    In addition, style computation tests are missing for text annotation

Rendering with master a7ece51

In the following, the text_annotations.bpmn file is used, but the same behaviour is observed with B.1.0 or B.2.0

01_master_a7ece51f52dbf06b9b6e0d970fa82dea2086fe96

Rendering with d30fc64

02_pr_d30fc64

Rendering with bc7c30f

Improvement proposal, this needs some reworks to match what I propose. We may increase the stroke value

03_commit_bc7c30fc

Rendering with bpmn-js@7.2.0 for reference

04_bpmn-js@7 2 0

src/component/mxgraph/config/ShapeConfigurator.ts Outdated Show resolved Hide resolved
Comment on lines 19 to 20
private readonly TEXT_ANNOTATION_BORDER_LENGTH = 20;
private readonly TEXT_ANNOTATION_MARGIN_FROM_SHAPE = 1;
Copy link
Member

Choose a reason for hiding this comment

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

❓ I guess we want a fixed size here. I am asking as we manage dynamic sizing for icons for now
See also #211
❓ I suggest to make this a style option for better extensibility. In addition, the border length seems to large and I would prefer to draw the stroke on shape bounds and use a margin for label (this seems to be the behaviour adopted by other BPMN vendors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes the styling here is fixed
improvements for label pushed

@aibcmars
Copy link
Contributor Author

aibcmars commented Jul 8, 2020

image

@aibcmars aibcmars requested a review from tbouffard July 8, 2020 14:51
@aibcmars aibcmars merged commit c25cdee into master Jul 8, 2020
@aibcmars aibcmars deleted the 386_render_text_annotation branch July 8, 2020 16:21
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] Render Text Annotation
3 participants