Skip to content

1513 dynamic resizing labels#2345

Open
timonlazarviadee wants to merge 14 commits intobpmn-io:developfrom
timonlazarviadee:1513-dynamic-resizing-labels
Open

1513 dynamic resizing labels#2345
timonlazarviadee wants to merge 14 commits intobpmn-io:developfrom
timonlazarviadee:1513-dynamic-resizing-labels

Conversation

@timonlazarviadee
Copy link

@timonlazarviadee timonlazarviadee commented Aug 26, 2025

Proposed Changes

This pull request concerns dynamic resizing of text fields. To this end, labels have been made resizable and code has been added that adjusts the height of the text to match the width.

Closes #1513

Visual demo

2025-08-26 09-48-39

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2025

CLA assistant check
All committers have signed the CLA.

});

function getTextHeightForTextAnnotation(event) {
return event.gfx.firstChild.lastChild.lastChild.y.animVal[0].value;
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how to make this look nicer. If you know another way, please show me with a pointer.

@barmac
Copy link
Member

barmac commented Sep 4, 2025

Hi, thanks for your contribution. We will look into this soon.

@barmac barmac added the needs review Review pending label Sep 4, 2025
@nikku nikku requested review from a team, barmac and philippfromme and removed request for a team September 10, 2025 06:56
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 10, 2025
@nikku nikku requested a review from jarekdanielak September 10, 2025 06:56
@barmac barmac added the needs review Review pending label Sep 10, 2025
@barmac
Copy link
Member

barmac commented Sep 10, 2025

Some interactions don't work properly or are not intuitive, so there's still some work ahead. I'd expect that the horizontal resize snaps to the text width. The label jump on vertical resize is also unintuitive.

Screen.Recording.2025-09-10.at.11.43.31.mov

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

Please add a test for horizontal resize as it's clearly not working correctly. The feature is useful, but we need to make sure it works correctly.

@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 10, 2025
@barmac barmac added the backlog Queued in backlog label Sep 11, 2025
@bpmn-io-tasks bpmn-io-tasks bot removed the backlog Queued in backlog label Nov 17, 2025
@barmac barmac added the needs review Review pending label Nov 21, 2025
@barmac
Copy link
Member

barmac commented Nov 24, 2025

@timonlazarviadee Multiple test cases are failing right now. Please check.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

CI needs to be fixed yet.

@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 24, 2025
@jarekdanielak
Copy link
Contributor

Hey @timonlazarviadee! This looks cool, sorry that we've kept you waiting for so long. Let's try to get this ready to merge...

It seems your branch is quite old, and we've recently added a feature related to external labels - label links, cf. #2328

Would you mind rebasing this branch on top of the latest develop to see how both features work together?

@timonlazarviadee timonlazarviadee force-pushed the 1513-dynamic-resizing-labels branch from 2f393e8 to 31ee9c8 Compare March 3, 2026 11:38
@timonlazarviadee timonlazarviadee requested a review from a team March 3, 2026 11:38
@jarekdanielak
Copy link
Contributor

@timonlazarviadee why does the PR change the code related to Text Annotations? I can break some of the text annotations behaviors now that works without your changes, cf. overlapping of text and connection line below.

I would be much easier to review and test your changes, if they were better isolated.

image

@timonlazarviadee
Copy link
Author

@timonlazarviadee why does the PR change the code related to Text Annotations? I can break some of the text annotations behaviors now that works without your changes, cf. overlapping of text and connection line below.

I would be much easier to review and test your changes, if they were better isolated.

image

I expected that TextAnnotations would be treated dynamically as labels. If you prefer the previous behavior, I can exclude TextAnnotations from this logic.

To better centralize the code, I would suggest extracting the resizing logic currently located in BpmnRenderer.js into a separate module, for example TextLayoutService.js. Methods such as renderLabel, renderEmbeddedLabel, and similar functions could then be handled within this service.

Do you think this approach would improve the structure and maintainability of the code?

To proceed with the implementation of this feature, I would also like to define a clear Definition of Done. In your opinion, what should this include, @jarekdanielak?

@timonlazarviadee timonlazarviadee force-pushed the 1513-dynamic-resizing-labels branch from 31ee9c8 to 71601c4 Compare March 9, 2026 12:37
@jarekdanielak
Copy link
Contributor

jarekdanielak commented Mar 9, 2026

Alright, let's start one step at a time, by refactoring your BpmnRenderer changes to better fit into our existing infrastructure.

We already have an utility for labels: lib/util/LabelUtil. All the helper functions for getting and setting the bounds, calculating text height, etc. should go there. Since text annotation uses renderLabel under the hood, it can also reuse those helpers.

WDYT?

@timonlazarviadee
Copy link
Author

Sounds good to me.

@timonlazarviadee
Copy link
Author

I rearranged the code to the LabelUtil.ts

@timonlazarviadee timonlazarviadee force-pushed the 1513-dynamic-resizing-labels branch from 64850a7 to e4d2cae Compare March 16, 2026 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add resizing for all text elements as for TextAnnotation

4 participants