-
Notifications
You must be signed in to change notification settings - Fork 493
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 process applications feature #4762
base: develop
Are you sure you want to change the base?
Conversation
@@ -1136,8 +1134,7 @@ export class App extends PureComponent { | |||
if ( | |||
activeTab !== prevState.activeTab || | |||
tabs !== prevState.tabs || | |||
layout !== prevState.layout || | |||
endpoints !== prevState.endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be used (again) once we support "endpoints" for deployment.
Why would we remove it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Of course fine, if we consider to store the endpoints in a different place, once supported).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because it's not used at all at the moment and was probably meant to be removed. I would rather reintroduce it again in the context of adding a feature that requires it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just have to be aware that this is "the data model" that was previously used. We can remove it, but then we'll not remember the data model once we re-introduce an endpoints feature.
@philippfromme Please add "steps to try out" to this PR, so @lmbateman and others can give it a run (to understand the UX we propose). |
cbd00bf
to
3714f6a
Compare
5350340
to
cbc64dc
Compare
cbc64dc
to
3c7d415
Compare
You mean Close process application would close all files belonging to the open process application? Interesting idea I haven't thought about. 🤔 |
Exactly. 😊 |
When I try to deploy a file from a process application, the modeler just reloads. Same thing happens when I save a file. Both only happens to files in a process application, the rest works normally. Screen.Recording.2025-01-15.at.11.56.30.mov |
This happens becasue the modeler reloads when something in |
I've added the |
70f0555
to
1653967
Compare
This comment was marked as resolved.
This comment was marked as resolved.
I'll try to help with this one. |
This comment was marked as resolved.
This comment was marked as resolved.
@philippfromme were you able to run a local build of this? So the one from On my machine, the build job results in the source code of To verify the build, besides running the app, you can execute this command for MacOS: |
Creating a variable on any task in BPMN model with Variables panel open (or opening it after) crashes. Screen.Recording.2025-01-15.at.17.36.50.mov |
@barmac I was able to successfully build locally and run the app. Hit me up if you want to test stuff. |
OK so it seems to be my local configuration problem. It's not a blocker in that case. |
Converted back to draft. Based on the discussion with @barmac I will make some changes to the implementation:
Miro: https://miro.com/app/board/uXjVPmTBc04=/?share_link_id=716131412171 |
@philippfromme Let's look into this today so that we can merge for the current release. I checked out the UI today, and I think having "New process application" on top of |
1653967
to
ca50db7
Compare
I haven't had time to do a full review, but here are some initial thoughts/findings:
|
@lmbateman Good feedback. I agree, creating a new process application leaves you with no indication whether it worked or not. We could show a success notification. When creating a new process application only directories can be selected, not files. We could make this more forgiving, allow files as well and then use the parent directory of the file (select foo/bar/baz.bpmn - foo/bar/.process-application will be created). Showing the directory name in the popup is something I'll try. |
ca50db7
to
c3b2ebe
Compare
@philippfromme I think a success notification would work, maybe with the process application name (e.g., "Successfully created process application [PA Name] with N files." I like the idea of being able to open a PA by selecting a file (if one exists) -- I think it will be rare for users to open a process application and not open a file to work on -- but would be interested to hear feedback from the rest of the team. I'm hoping to do a more thorough review this week. |
Unfortunately offering both file and directory selection doesn't seem to work (cf. electron/electron#26885). At least on Windows it doesn't. 😞 |
c3b2ebe
to
0e654fc
Compare
Proposed Changes
Backend
file-context
feature.bpmn
,.dmn
,.form
,.process-application
)Frontend
ProcessApplications
feature to open and close process applications automatically.process-application
file will be createdTry it out
I've added an example process application: a354d21
Open any file in this directory to see the process application.
To run the Camunda Modeler try
npm install && npm start
Follow-ups
References
feature to get reference from and to resourcesAutoCompletions
feature to get autocompletionsCloses #4666
Closes #4675
Closes #4687
Related to #4676
Related to #4664
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}