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

fix: make name change behavior not break on name change #917

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

barmac
Copy link
Member

@barmac barmac commented Nov 8, 2024

Proposed Changes

Screen.Recording.2024-11-08.at.10.24.04.mov

We've tested the label update but not the direct name change with modeling.updateProperties. Now we test for both.

Related to bpmn-io/dmn-js-properties-panel#103
Related to camunda/camunda-modeler#4684

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}

@barmac barmac requested a review from nikku November 8, 2024 09:25
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 8, 2024
@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

After a second look at the name change behavior, I am not super happy about having two components doing the same thing (boxed expression and shared). However, for this fix I decided to only make a small change to avoid a potential bug.

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

Once merged, this will be released as 16.8.2

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

We missed updating the name attribute via modeling APIs and I see this is now covered.

Relevant follow-up: Will it break if we update moddle properties?

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

We missed updating the name attribute via modeling APIs and I see this is now covered.

Relevant follow-up: Will it break if we update moddle properties?

Hmm I think it will just not trigger. We could add it too.

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

But I'd do it in a separate PR. Ignoring an update is not breaking the editor.

Co-authored-by: Nico Rehwaldt <nico.rehwaldt@camunda.com>
@barmac barmac merged commit 4b4fd90 into 16.x Nov 8, 2024
6 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 8, 2024
@barmac barmac deleted the make-name-change-behavior-work-with-update-properties branch November 8, 2024 09:41
@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

Published as v16.8.2

@barmac barmac mentioned this pull request Nov 8, 2024
4 tasks
@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

Hmm this job should not have run at all: https://github.com/bpmn-io/dmn-js/actions/runs/11739770879
We need to fix it later on.

@nikku
Copy link
Member

nikku commented Nov 8, 2024

I think it would be reasonable to check for @latest tag and if that does not exist (because a release is a backport) the job is fine to fail.

By the way, your release broke latest, it is now back to 16.x, cf. npm.

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

Thanks for notice! I'll fix that

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

This will be fixed via v17.0.1 release.

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

Hmm I can't release it because tests fail due to f402e25. I will have a look again after lunch.

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

As a temporary fix, I fixed the latest for dmn-js. The subpackages will be fixed with the v17.0.1

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

So the test is now broken although its expects are logical. I debugged the test case, and while we set incorrect classes in the code, the actual position of the simple mode button is correct (top-right but with .bottom.right classes).

@barmac
Copy link
Member Author

barmac commented Nov 8, 2024

BTW the classes are not used for anything apart from the assertions, so we could as well remove them. I will double-check.

@barmac barmac mentioned this pull request Nov 8, 2024
4 tasks
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.

2 participants