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(cloud-templates): conditions on dropdown choices #14

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

smbea
Copy link
Contributor

@smbea smbea commented Aug 24, 2023

Depends on camunda/element-templates-json-schema#107
Related to camunda/camunda-modeler#3682

Fell free to use the gitlab connector in the demo to test it, it was the perfect use case:

Screen.Recording.2023-08-25.at.13.47.33.mov

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Aug 24, 2023
@smbea smbea marked this pull request as ready for review August 25, 2023 12:57
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Aug 25, 2023
@smbea smbea requested review from a team, philippfromme and marstamm and removed request for a team August 25, 2023 12:58
Copy link
Collaborator

@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.

It seems that we do not handle cleanup/chained conditions properly. In the example below, the options for "Create a release" are still present even when the operation type can only be "Issue" related.

What was our previous behavior for these kind of "dependent" optional values? I would assume that we clean up the XML when an option is hidden again, so the properties panel reflects the XML and not simply hide the UI but keep the values in the XML.

Recording 2023-08-25 at 16 37 52

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 25, 2023
@smbea
Copy link
Contributor Author

smbea commented Aug 25, 2023

Ah, maybe it isn't as easy as I made it. I will leave this "in progress" and check this when I'm back from FTO (see if I can fix it easily or just leave it since it's a spring cleaning topic)

@nikku nikku marked this pull request as draft August 28, 2023 14:23
@nikku nikku added the ready Ready to be worked on label Aug 28, 2023 — with bpmn-io-tasks
@nikku nikku removed the in progress Currently worked on label Aug 28, 2023
@smbea
Copy link
Contributor Author

smbea commented Sep 12, 2023

In theory, the conditions mechanism should work. What I suspect might be happening is the timing between re-renders (and therefore setting the dropdown values in xml) and the conditions checks is off. I move this to backlog and pick in the next spring cleaning or such

@smbea smbea added the backlog Queued in backlog label Sep 12, 2023 — with bpmn-io-tasks
@smbea smbea added spring cleaning Could be cleaned up one day and removed ready Ready to be worked on labels Sep 12, 2023
@smbea
Copy link
Contributor Author

smbea commented Oct 10, 2023

I found that the problem here is with the xml property cleanup. When changing a parent property, the child dropdown retains the old XML value, which isn't even in the current options:

Screen.Recording.2023-10-10.at.09.49.35.mov

This happens because we trigger xml updates when updating element templates, which is not the case here

@smbea
Copy link
Contributor Author

smbea commented Oct 10, 2023

This theoretically actually acts according to our element template expectations, which is that we don't override the existing value. Let's imagine if this were a string value:

Screen.Recording.2023-10-10.at.16.35.07.mov

Makes sense to keep that. This is the same thing that happens with dropdown. The issue is that the underlying value does not match the options presented in the UI so it looks like a bug. We can change it for these cases, so that it doesn't look broken (e4e0ef5):

Screen.Recording.2023-10-10.at.16.37.58.mov

But it would go against our general way of doing it. Any thoughts @marstamm ?

Note: if this isn't a simple choice moving forward, I will drop it as spring cleaning

@nikku
Copy link
Member

nikku commented Jan 11, 2024

Potentially related is this bug report, where we see wired behavior with dropdowns (and their initial value not being applied): https://github.com/camunda/web-modeler/issues/7649#issuecomment-1884983039.

@smbea smbea removed their assignment Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Queued in backlog spring cleaning Could be cleaned up one day
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants