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

Explain output mapping outcome in the tooltip #4889

Open
barmac opened this issue Mar 10, 2025 · 9 comments
Open

Explain output mapping outcome in the tooltip #4889

barmac opened this issue Mar 10, 2025 · 9 comments
Assignees
Labels
fixed upstream Requires integration of upstream change good first issue Good for newcomers spring cleaning Could be cleaned up one day

Comments

@barmac
Copy link
Collaborator

barmac commented Mar 10, 2025

What should we do?

Adjust the outputs tooltip to explain the outcome of adding mapping:

Image

Consider suggestions:

  • All returned variables will become local task variables and only the mapped ones will be merged into the global scope.
  • Add an output mapping to control what is merged back into the process scope.

The point is that leaving outputs empty means that all task variables are implicitly merged into the process scope. If you add a single mapping, remaining variables are discarded.

Note also that it's different for call activity where user can specify whether they want to merge the variables.

Why should we do it?

Explain basics in the Modeler to prevent context switching.

Reported at https://camunda.slack.com/archives/C0693F1NFK5/p1741593077729079

@barmac barmac added backlog Queued in backlog good first issue Good for newcomers spring cleaning Could be cleaned up one day labels Mar 10, 2025
@barmac
Copy link
Collaborator Author

barmac commented Mar 10, 2025

BTW the current tooltip is also misleading as the variables are not merged into the global scope if the task is in a subprocess. It's only that they are no longer in the local task scope anymore.

@barmac barmac added the ready Ready to be worked on label Mar 10, 2025 — with bpmn-io-tasks
@barmac barmac removed the backlog Queued in backlog label Mar 10, 2025
@barmac barmac added the backlog Queued in backlog label Mar 10, 2025 — with bpmn-io-tasks
@barmac barmac removed the ready Ready to be worked on label Mar 10, 2025
@nikku nikku added ready Ready to be worked on and removed backlog Queued in backlog labels Mar 10, 2025
@nikku
Copy link
Member

nikku commented Mar 10, 2025

Let's tackle this, probably my proposal is good enough (updated with suggestions from #4889 (comment)):

Add output mappings to control which variables are merged back into the process scope.

We want to also tackle the input mappings accordingly.

Add input mappings to control what is passed to the activity as local variables.

@barmac
Copy link
Collaborator Author

barmac commented Mar 11, 2025

CC @lmbateman

@Buckwich Buckwich added the in progress Currently worked on label Mar 13, 2025 — with bpmn-io-tasks
@Buckwich Buckwich removed the ready Ready to be worked on label Mar 13, 2025
Buckwich added a commit to bpmn-io/bpmn-js-properties-panel that referenced this issue Mar 14, 2025
@lmbateman
Copy link

Sorry, sorry, I missed this earlier.

  • Would it be better to mention variables in the output mappings tooltip? Something like, "Add output mappings to control which variables are merged back into the process scope."
  • The input text looks OK to me, although I wonder if it makes sense to change "provided" to "passed".
  • This might be a good time to move the links into the text, maybe around "input mappings" and "output mappings"; wdyt?

@nikku
Copy link
Member

nikku commented Mar 14, 2025

  • Would it be better to mention variables in the output mappings tooltip? Something like, "Add output mappings to control which variables are merged back into the process scope."

Interesting question. The mapping can be used to pick variables, but it can also be used to transform / project:

isOver_18 = applicant.age > 18

I updated my proposal, incorporating your hints.

@Buckwich
Copy link
Member

@lmbateman You mean like this?
Image

My only concern with that is that its inconsistent with the other texts, so we should update them as well. And translations could be a bit more tricky

@nikku
Copy link
Member

nikku commented Mar 14, 2025

My only concern with that is that its inconsistent with the other texts

What would be inconsistent, because of the inline link? I think it is fine to keep Learn more, removing it was probably not @lmbateman's intention. What would be valuable though is to update the message, as per #4889 (comment).

You can find the updated message that incorporates @lmbateman's suggestions here.

@lmbateman
Copy link

lmbateman commented Mar 14, 2025

I was suggesting removing "Learn more", in accordance with our new guidelines for documentation links in UI text. But Simon is right; we should probably do that more broadly rather than introducing it tooltip by tooltip. And I don't think we need two links to the same place, so let's leave "Learn more" for now.

@nikku's updated proposal looks good to me. Thanks!

@Buckwich
Copy link
Member

Buckwich commented Mar 14, 2025

I updated the PR1115 with the new texts, without changes to learn more. Thanks @nikku & @lmbateman .

Buckwich added a commit to bpmn-io/bpmn-js-properties-panel that referenced this issue Mar 14, 2025
Buckwich added a commit to bpmn-io/bpmn-js-properties-panel that referenced this issue Mar 14, 2025
@Buckwich Buckwich added the fixed upstream Requires integration of upstream change label Mar 14, 2025 — with bpmn-io-tasks
@Buckwich Buckwich removed the in progress Currently worked on label Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed upstream Requires integration of upstream change good first issue Good for newcomers spring cleaning Could be cleaned up one day
Projects
None yet
Development

No branches or pull requests

4 participants