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

Bump modeling dependencies #4154

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Bump modeling dependencies #4154

merged 5 commits into from
Feb 26, 2024

Conversation

nikku
Copy link
Member

@nikku nikku commented Feb 23, 2024

Closes #4118
Closes #4037

Related to #3622
Related to #4062

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Feb 23, 2024
@nikku nikku requested a review from barmac February 23, 2024 17:44
@nikku nikku added this to the M74 milestone Feb 23, 2024
@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

The documentation link from HTTL rule leads to a non-existing page: https://docs.camunda.io/docs/next/components/modeler/reference/modeling-guidance/rules/history-time-to-live

Note that the URL generated from https://github.com/camunda/camunda-docs/pull/3324/files will be different (httl-required vs. history-time-to-live).

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

#4118 is fixed 👍

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

#4037 is fixed 👍

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

For #3622, I will add a template to dev resources which uses new properties.

@nikku
Copy link
Member Author

nikku commented Feb 26, 2024

#4154 (comment)

Yes, that is intentional. What we use is the final URL, and the documentation needs to be fixed.

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

image

I cannot set a default number value to number property.

@nikku
Copy link
Member Author

nikku commented Feb 26, 2024

I cannot set a default number value to number property. (ref)

Let's consider if this is the desired behavior, or = 100 would be the proper way to set a default.

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

The dynamic ID check is not supported and triggers undesired suggestion in VS Code:

Screen.Recording.2024-02-26.at.10.42.41.mov

We should probably switch to 100% programmatic support.

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

Combination of type: Boolean, value: true, feel: required blows up the FEEL entry:

Screen.Recording.2024-02-26.at.10.46.15.mov

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

type: Number, feel: static works incorrectly when changing values with increment/decrement:

Screen.Recording.2024-02-26.at.10.48.24.mov

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

type: Number, feel: static works incorrectly when changing values with increment/decrement:

Screen.Recording.2024-02-26.at.10.48.24.mov

This applies also to feel: optional though it seems to jump less.

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

Such condition is never fulfilled regardless of the field value:

    {
      "type": "Text",
      "value": "displayed when static is 100",
      "binding": {
        "name": "text",
        "type": "zeebe:property"
      },
      "condition": {
        "property": "number-static",
        "equals": 100
      }
    }

I have to use string value to make it work.

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

type: Boolean, feel: optional does not trigger FEEL editor

Screen.Recording.2024-02-26.at.10.54.39.mov
    {
      "id": "boolean-optional",
      "type": "Boolean",
      "value": true,
      "label": "boolean-optional",
      "binding": {
        "name": "boolean-optional",
        "type": "zeebe:property"
      },
      "feel": "optional"
    }

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

@nikku I've pushed my dev template for number and boolean values. We should fix the bugs I found prior to the release or disable the feature.

@nikku
Copy link
Member Author

nikku commented Feb 26, 2024

To better understand the impact: Is this breaking existing functionality (if no Boolean or Number inputs are used)? For now it looks to me like we can continue (no need to disable).

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

It's not breaking. Only a new feature is broken. Sure we can merge and fix this later. It's not a blocker for the merge but rather for the release.

@barmac
Copy link
Collaborator

barmac commented Feb 26, 2024

What is troubling is that the Windows build is consistently failing :/

nikku and others added 2 commits February 26, 2024 11:47
deps: update to `bpmn-js-element-templates@1.14.0`

  Closes #4037
  Related to #3622

deps: update to `bpmn-js@17.0.2`

deps: update to `diagram-js@14.1.0`
@nikku
Copy link
Member Author

nikku commented Feb 26, 2024

It's not breaking. Only a new feature is broken. Sure we can merge and fix this later. It's not a blocker for the merge but rather for the release.

Let's consider if we announce it or not. That is all, I think.

I updated this PR to not indicate that #3622 is closed. Could you comment on the issue regarding the remaining topics to handle?

@nikku
Copy link
Member Author

nikku commented Feb 26, 2024

Let's get this PR merged, and follow-up afterwards.

@nikku nikku merged commit 1db4ef3 into develop Feb 26, 2024
10 of 12 checks passed
@nikku nikku deleted the bump-modeling-dependencies branch February 26, 2024 11:04
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Feb 26, 2024
@nikku
Copy link
Member Author

nikku commented Feb 26, 2024

Moved #3622 back to ready as it is clearly not fixed.

@marstamm
Copy link
Member

Thank you for the thorough integration testing. I gave all reported issues an initial evaluation here.

type: Boolean, feel: optional does not trigger FEEL editor

@barmac , I could not reproduce this issue, is there anything I am missing when trying to reproduce it?

Recording 2024-02-28 at 11 50 29

@barmac
Copy link
Collaborator

barmac commented Feb 28, 2024

It looks like this is an edge case, but the UI jumps/flickers if you click a lot:

Screen.Recording.2024-02-28.at.14.54.24.mov

@nikku
Copy link
Member Author

nikku commented Feb 28, 2024

Looks like a bug to me. Clicking the FX toggle does not properly prevent the click interaction, temporarily triggering checking the checkbox (via standard browser "check if label for checkbox is clicked" interaction.

@marstamm
Copy link
Member

Clicking the FX toggle does not properly prevent the click interaction, temporarily triggering checking the checkbox

I agree, but it does not prevent you from toggling as initially reported. This is also an issue for boolean toggles in form-js, so it needs to be fixed in the properties-panel. I'll create some GH issues for what was reported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants