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

feat: integrate create/append anything #2848

Closed
wants to merge 2 commits into from

Conversation

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Mar 26, 2022
@nikku nikku force-pushed the 2820-integrate-create-append-anything branch from 1b45f38 to ba54097 Compare March 26, 2022 09:51
@nikku nikku force-pushed the 2820-integrate-create-append-anything branch 2 times, most recently from 2aee76a to 7e8df4e Compare March 26, 2022 18:34
@nikku nikku force-pushed the 2820-integrate-create-append-anything branch from 7e8df4e to c3d95fb Compare March 26, 2022 18:38
@nikku nikku marked this pull request as ready for review March 26, 2022 21:09
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Mar 26, 2022
@pinussilvestrus
Copy link
Contributor

Although I'm pretty hyped about this feature (behind a feature toggle or not), shouldn't we have a base test coverage for the extension before merging it?

@nikku
Copy link
Member Author

nikku commented Mar 27, 2022

Shouldn't we have a bpmn-io/bpmn-js-connectors-extension#12 for the extension before merging it?

Absolutely. More importantly though we need to validate that this goes into the right direction (also feature wise) and decide if this should or should not be 5.0.0 scope. This PR is the foundation for that discussion 🙂.

@nikku nikku marked this pull request as draft March 27, 2022 20:53
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Mar 27, 2022
@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

@MaxTru @christian-konrad I've promised a little update what/if we can deliver the new modeling UX with the Camunda Modeler 5.0.

My assessment is, yes, with a little work remaining to properly integrate it with Camunda Modeler, potentially even with 5.0.0 (cf. list of know issues). We should however not ship the limited "append connector" variant, but a scalable create/append anything experience showcased in this PR.

Please take a few minutes to review what is inside this Pr and let us follow up on it (later today!).


See also bpmn-io/bpmn-js-connectors-extension#20

See also connectors solution architecture.

@MaxTru
Copy link
Contributor

MaxTru commented Mar 28, 2022

I tested the PR. Please notice that I looked at this without the connectors glasses on. So we should IMO ensure, that this adds value in all use cases.

  • I really really like this feature and think it improves our existing create/append experience. Hence I personally would like to have it asap :-) <3
  • For me must-dos prior to integration (on top of the existing ones are):
    • Keyboard shortcuts are active when typing the menu (e.g., s starts snipping tool)
  • What I would revisit optionally is:
    • The * icon is "okay", but maybe Design could have a look (my personal impression)
    • Why do the Templates elements not have a symbol preview?
    • I think we need to test, how create/append templates for element templates for non-activities (e.g., events, even SubProcess is possible) behaves
    • For C7 no templates can be created / appended (for C8 it works)

@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

The * icon is "okay", but maybe Design could have a look (my personal impression)

Also feedbacked by @CatalinaMoisuc earlier today.

@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

Feedbacked by @SebastianStamm today:

What about making the append / replace options entirely dynamic?

Tracked this via bpmn-io/bpmn-js-create-append-anything#3. I think it requires a bunch of additional thoughts.

@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

For C7 no templates can be created / appended (for C8 it works)

@MaxTru This is intentional, as we decided to not implement ElementTemplates#createElement for C7. I don't believe this to be a blocker, too. But happy to discuss.

See also bpmn-io/bpmn-js-connectors-extension#17 (comment).

@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

The * icon is "okay", but maybe Design could have a look (my personal impression)

I second this. My initial proposal was a simple +. Alternatively, a ... "append/create something else" would do, too.

@christian-konrad
Copy link
Contributor

If we decide on and add it like this, we should measure it's adoption/usage right from the beginning:

  • How many model elements were created the new way vs. the "old" way?

We can therefore decide in the future, if

  • We need to course-correct user behaviour
  • We have to tweak the UX a little bit more
  • We may drop the "legacy" way of appending (if "append only" : "legacy append" ratio is suitable)

@ingorichtsmeier
Copy link

Hi @nikku,

This is intentional, as we decided to not implement ElementTemplates#createElement for C7.

What would be the effort, in case a customer would ask for this feature? S, M, XL or XXXXL?

@MaxTru
Copy link
Contributor

MaxTru commented Mar 28, 2022

For C7 no templates can be created / appended (for C8 it works)

@MaxTru This is intentional, as we decided to not implement ElementTemplates#createElement for C7. I don't believe this to be a blocker, too. But happy to discuss.

See also bpmn-io/bpmn-js-connectors-extension#17 (comment).

Agreed. Updated my comment.

@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

@ingorichtsmeier How can the customer ask? She does not know it yet, right? 😉

The effort would be M. As discusse with @pinussilvestrus there is quick hacks / workarounds that we can do to not fully rebuild it from scratch for C7 (which has a large element template supported feature set).

@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

#2848 (comment)

@christian-konrad Agreed, not sure though if we have the numbers for now ("old state"). What I'd suggest is that we track adoption of the bits added.

How many model elements were created the new way vs. the "old" way?

This would be "how many nodes did we model new way vs. old way", where new way is:

  • Search used in create/replace/append menu
  • Create anything / append anything used in the first place

@nikku nikku added the backlog Queued in backlog label Mar 28, 2022 — with bpmn-io-tasks
@nikku nikku removed the in progress Currently worked on label Mar 28, 2022
@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

Quick summary of a session with @MaxTru and @christian-konrad:

  • Decision (@christian-konrad): We'll not ship this with 5.0 for different reasons; one is that we want to closely track feature adoption.

  • In the future we'll likely need to consider merging connectors focused UI and create/append anything. One option is to have both on the canvas:

image

I'm leaving this PR open; to be potentially picked up in Q2 or closed if priorities change.

@nikku
Copy link
Member Author

nikku commented Mar 28, 2022

Regarding #2848 (comment), this is a version that uses ... instead of *, a feedback that I've received a few times:

image

@christian-konrad
Copy link
Contributor

Found some issues:

  • While typing in the search bar, pressing keys like "e" moves the focus out of the bar and triggers the corresponding shortcut.
  • "Script Task" is missing in tasks and only appears when typing "script"
  • After appending a new dedicated task/element/connector, the text input overlay hides the icon, thus confuses the user (was this the right selection?), see Direct editing box hides underlying element bpmn-io/diagram-js-direct-editing#23

Additional feedback:

@nikku
Copy link
Member Author

nikku commented Mar 29, 2022

"Script Task" is missing in tasks and only appears when typing "script"

While experimental, this is a feature, not a bug. We encourage users to model what is reasonable and hide everything else behind search (for discovery). Same applies for example for manual tasks (discouraged; no-op) and inclusive gateway (wanky implementation semantics).

But generally speaking: Make things that we encourage to user easy to discover; make everything else discoverable via "second read". That is the direction should go.

For example we could unfeature blank tasks and intermediate events in the same spirit to encourage "modeling for execution".

@nikku
Copy link
Member Author

nikku commented Mar 29, 2022

Dots ... are now the icon for "appending / creating anything":

capture IWCm2c_optimized

@MaxTru
Copy link
Contributor

MaxTru commented Mar 29, 2022

Dots ... are now the icon for "appending / creating anything":

capture IWCm2c_optimized

👍 for ...
👎 for *

@nikku nikku removed their assignment Aug 26, 2022
@nikku
Copy link
Member Author

nikku commented May 2, 2023

Integrated.

@nikku nikku closed this May 2, 2023
@bpmn-io-tasks bpmn-io-tasks bot removed the backlog Queued in backlog label May 2, 2023
@nikku nikku deleted the 2820-integrate-create-append-anything branch May 2, 2023 12:42
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.

Make improved modeling UI available in Modeler
5 participants