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 _Binding_ entry to busines rule task, call activity and user task #1067

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Jul 25, 2024

Proposed Changes

  • adds a Binding entry to business rule tasks implemented as DMN, call activities and user tasks with linked form

Check out the branch and run npm start to test.

Business rule tasks

image

Call activities

image

User tasks

image

Depends on camunda/camunda-bpmn-js-behaviors#78
Depends on bpmn-io/properties-panel#376

Tooltip

image

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}

Related to camunda/camunda-modeler#4385

@nikku
Copy link
Member

nikku commented Jul 25, 2024

I'm cross posting a concern from the upstream PR here:

The design sketches seem to imply we now start to recommend best actions for users (in italic). I don't like this for two reasons:

  • We don't usually give best practices in the tooltips, but are descriptive and add a clear call to action ("learn more"). The tooltips should give people enough context to understand what is happening, but not do full education.
  • The good practices are misleading. If I'm developing a process application (which is the default in the future), then from the start deployment is the safe and recommended option. This becomes especially evident the design sketches propose versionTag to be the best solution for production environments. Which is just wrong (cf. above). Inside of a process application deployment is always the best and safest choice, in my opinion.

My proposal is that we leave it to the documentation to give advice, and ensure that we properly apply sensible defaults, based on the context (ref). At best a user should not need to touch this property at all in order to be successful.


CC @lmbateman @philippfromme @RomanKostka

@philippfromme
Copy link
Contributor Author

@nikku I drastically simplified the tooltip.

@nikku
Copy link
Member

nikku commented Jul 25, 2024

@philippfromme Don't get me wrong: A short and concise factual description, if correct can help. I'd just want to keep out any detailed "recommendation" out of these tooltips.

@lmbateman
Copy link

I think we should either leave out "The default is latest" from the tooltip, or add a reason why. If they haven't changed it, the default should be obvious, so that sentence isn't helpful; and if they have, then it would be useful to know why the default is what it is. Also, based on Nico's comments, is latest the best default?

Ideally, we should keep the user in the application, rather than forcing them to go visit the documentation. To that end, I'd prefer a tooltip with a little more explanation, because the meanings and benefits of the options may not be obvious from their titles. How about a middle-of-the-road version along these lines? We could probably tighten the text up even more. Conversely, if we can nail down the right recommendations, I'd be OK with adding them back in.

image

@nikku
Copy link
Member

nikku commented Jul 25, 2024

@lmbateman I think your proposal works well; I'd add deployed to the mix. Some minor suggestions:

  • Latest = most recent **deployed** version
  • Deployment = Uses the linked resource in the same deployment.
  • VersionTag = Uses the latest deployed version with a specific version tag.

@philippfromme
Copy link
Contributor Author

The Version tag option is not going to be supported according to https://github.com/camunda/product-hub/issues/1920#issuecomment-2168146584 so I didn't implement that. I assume, this would be added as part of https://github.com/camunda/product-hub/issues/435.

@philippfromme philippfromme force-pushed the binding-type branch 2 times, most recently from 5492b34 to b125eb3 Compare July 29, 2024 12:06
@philippfromme philippfromme marked this pull request as ready for review July 29, 2024 12:10
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jul 29, 2024
@philippfromme
Copy link
Contributor Author

I've left out the documentation link since there is none yet.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

I prefer the "uses the most recent..." proposal to "binds to..." in the tooltip, but this is good enough. ✅ from me.

@philippfromme
Copy link
Contributor Author

Ok, adjusted.

@philippfromme philippfromme merged commit 63ccae1 into main Jul 29, 2024
8 checks passed
@philippfromme philippfromme deleted the binding-type branch July 29, 2024 14:00
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jul 29, 2024
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.

4 participants