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

Optional zeebe:input and zeebe:output #565

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Conversation

pinussilvestrus
Copy link
Contributor

Closes #559

Script to try it out

npx @bpmn-io/sr bpmn-io/bpmn-js-properties-panel#559-optional-inputs-outputs -c "npm run start:cloud-templates"

Kapture 2022-02-01 at 16 03 57

@pinussilvestrus pinussilvestrus requested review from a team, marstamm and smbea and removed request for a team February 1, 2022 15:05
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Feb 1, 2022
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.

I can confirm it works, I did not check the implementation.

@pinussilvestrus pinussilvestrus force-pushed the 559-optional-inputs-outputs branch from b64b43a to a108a24 Compare February 3, 2022 15:31
@pinussilvestrus
Copy link
Contributor Author

Please note, that optional is a normal property here, no constraint, since it has no real UI validation usage.

{
  "value": "input-1-source",
  "type": "String",
  "optional": true,
  "binding": {
    "type": "zeebe:input",
    "name": "input-1-target"
  }
}

Comment on lines 286 to 287
shouldUpdate(newPropertyValue, newProperty) ||
(isOptional && propertyChanged(oldInputOrOutput, oldProperty))
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to wrap my head around what's checked here. My assumption would be we always want to update when the property changes and when it is not optional.

I'm wondering if isOptional is needed here. shouldUpdate returns false when the value is empty and optional. For the second condition, isOptional should always be true when we reach it, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right, the second isOptional does not make sense here, also syntax-wise: If the property is optional (and has an empty value), the old input/output should be cleaned up (unless changed).

@pinussilvestrus pinussilvestrus force-pushed the 559-optional-inputs-outputs branch from a108a24 to 1042658 Compare February 7, 2022 10:14
Copy link
Contributor

@marstamm marstamm left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@fake-join fake-join bot merged commit 1b9317e into master Feb 7, 2022
@fake-join fake-join bot deleted the 559-optional-inputs-outputs branch February 7, 2022 10:28
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Feb 7, 2022
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 ability to create optional inputs & outputs via cloud element templates
3 participants