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

Add element templates to replace menu #204

Closed
Tracked by #1627
smbea opened this issue Nov 18, 2022 · 12 comments · Fixed by #207
Closed
Tracked by #1627

Add element templates to replace menu #204

smbea opened this issue Nov 18, 2022 · 12 comments · Fixed by #207
Assignees

Comments

@smbea
Copy link
Contributor

smbea commented Nov 18, 2022

What should we do?

Add element templates to replace menu options

Why should we do it?

In the context of support replace anything, see bpmn-io/bpmn-js#1627

@smbea smbea self-assigned this Nov 18, 2022
@smbea smbea added the ready Ready to be worked on label Nov 18, 2022
@smbea
Copy link
Contributor Author

smbea commented Nov 18, 2022

My current implementation for this consists in adding a replace menu provider that feeds element templates as menu entries (like "ElementTemplatesProvider").

@smbea
Copy link
Contributor Author

smbea commented Nov 28, 2022

WIP: https://github.com/camunda/camunda-bpmn-js/tree/204-add-replace-anything


The end goal of replace with templates:

In bpmn-js-connectors-extension, the replace is populated with templates which apply to the current selected element (see https://github.com/bpmn-io/bpmn-js-connectors-extension/blob/6ac379adf9b366eed0e37d9986d9762c1964f946/src/replace-menu/ReplaceMenu.js#L51).

Example: if default task is selected, only templates that have appliesTo: ["bpmn: Task"] appear.

To consider: allow replace with any templates? In the same context that we can replace a default task with a service task, does it make sense to allow replace of a default task with a service task template (replace element + apply template)?

@smbea smbea added in progress Currently worked on and removed ready Ready to be worked on labels Nov 28, 2022
@nikku
Copy link
Member

nikku commented Nov 29, 2022

@smbea A little summary of our chat (weekly) on this:

  • Elements not appearing if appliesTo: [ element not in list ] is expected behavior
  • For elements to appear (i.e. ServiceTask -> UserTask replacement) you have to appliesTo: [ bpmn:Task or bpmn:ServiceTask,bpmn:UserTask ]. elementType is the designated utility that allows you to define into which element type the element is changed during replace.

smbea added a commit that referenced this issue Nov 29, 2022
smbea added a commit that referenced this issue Nov 29, 2022
smbea added a commit that referenced this issue Nov 30, 2022
smbea added a commit that referenced this issue Nov 30, 2022
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending in progress Currently worked on and removed in progress Currently worked on needs review Review pending labels Nov 30, 2022
@smbea
Copy link
Contributor Author

smbea commented Nov 30, 2022

I have an initial draft for this in #207. Before getting into the nitty gritty, I would like to get some feedback on the high level structure/organisation of the feature. Does the location of the new provider added make sense? Should it be structured different?

@nikku
Copy link
Member

nikku commented Nov 30, 2022

@smbea I've commented on the PR.

The solution looks simple enough; let's make sure it plugs in correctly end-to-end, also including edge cases (service task backed template -> service task (unbind template)).

Also does it make sense to structure this in accordance with bpmn-js feature structure?

@smbea
Copy link
Contributor Author

smbea commented Nov 30, 2022

I missed that part indeed, will work on it now.

I think it makes sense to follow that structure (like ../features/replace). It's what we already do in our other tools and we already have the features dir in here.

smbea added a commit that referenced this issue Nov 30, 2022
@smbea
Copy link
Contributor Author

smbea commented Nov 30, 2022

I was working on allowing to replace back with plain element (service task backed template -> service task). But I am struggling to make sure the option is added back in the original position when working with the first task. This also appears as a bug on the connectors extension, example:

After applying a template to a Task, the Task replace option doesn't appear where it should be:

Screenshot 2022-11-30 at 21 17 23

@nikku
Copy link
Member

nikku commented Dec 1, 2022

From discussions with @marstamm when we fixed that issue I remember that we did the least effort thing to build it.

I cannot reproduce #204 (comment) on the connectors extension, unfortunately.

capture oLfjPO_optimized


What the extension implementation does is to try to insert the element in the original position, taking the next and previous elements as an anchor.

The element itself cannot be replaced, as it is filtered out at the core. We could decide to improve this in the core by making it pluggable or element template aware.

@marstamm
Copy link
Member

marstamm commented Dec 1, 2022

[FYI]: this is the commit that changed the unlink entry position: bpmn-io/bpmn-js-connectors-extension@0ec6a11

EDIT: Nico was faster and already added the relevant code in the original comment 😄

@smbea
Copy link
Contributor Author

smbea commented Dec 1, 2022

It works for middle positions like Service Task. I had an issue in trying to apply a template to Task. I tried with this template:

{
  "$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json",
  "name": "AppliesToTask",
  "id": "com.camunda.example.AwesomeTask",
  "appliesTo": [
    "bpmn:Task"
  ],
  "properties": [
    {
      "label": "Are you awesome?",
      "type": "Boolean",
      "value": true,
      "binding": {
        "type": "property",
        "name": "customProperty"
      }
    }
  ]
}

@nikku
Copy link
Member

nikku commented Dec 1, 2022

@smbea In this case the implementation shall hook the task up in front of the next element in the line. It may be broken, then we need to fix it.

My suggestion is: Let's incorporate it, build some basic test coverage around it, and investigate + fix all bugs we find in the process.

@smbea
Copy link
Contributor Author

smbea commented Dec 1, 2022

I have looked into the implementation and I understand why it happens. But I was testing out edge cases and it seemed like a bug to me so I wanted to be on the same page.

Since it works for most cases, I will do that: incorporate like this and then improve on top. Thanks for the insight!

smbea added a commit that referenced this issue Dec 1, 2022
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 1, 2022
nikku pushed a commit that referenced this issue Dec 2, 2022
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 2, 2022
smbea added a commit that referenced this issue Dec 2, 2022
nikku pushed a commit that referenced this issue Dec 2, 2022
@smbea smbea closed this as completed in #207 Dec 2, 2022
smbea added a commit that referenced this issue Dec 2, 2022
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Dec 2, 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 a pull request may close this issue.

3 participants