Closed AlexanderSkrock closed 5 months ago
@AlexanderSkrock Great to see you picking this up. You should be able to link both projects, this core + bpmn-js-properties-panel to test them together:
cd bpmn-js-properties-panel
npm link ../properties-panel
...
This pull request together with changes made in #1047 in bpmn-properties-panel should be good to go now, from my point of view.
One more question, I saw that we released properties-panel
version 3.18.2 already, so we would need to increment and also update it for bpmn-js-properties-panel
. Is this a step which is done automatically during the release process?
@AlexanderSkrock You'd want to rebase this PR on main before merge.
Once you did it then I can give it my review.
@AlexanderSkrock You'd want to rebase this PR on main before merge.
I checked once again, but it seems my commits were based on main's head already.
Will look into your changes today. Thanks already for your work :clap:
After some distractions I hope I find the time to finalize my review + testing tomorrow.
I found the time to review the change, along with https://github.com/bpmn-io/bpmn-js-properties-panel/pull/1047.
A few notes:
List#compareFn
is used along with sorting to implement sort features; without sorting there is no necessity for compareFn
, we want to ditch it which I sketched in https://github.com/bpmn-io/bpmn-js-properties-panel/pull/1047/commits/d883a0fcc3cb19ddf422b1ab3a86bc1595cfb28a.Integration testing https://github.com/bpmn-io/bpmn-js-properties-panel/pull/1047 I found a bunch of places where auto focus is now broken; we want to ensure that items are added last to the list (no magic), and focused regardless, sketched for Execution Listeners > Execution Listener > Field injections
via https://github.com/bpmn-io/bpmn-js-properties-panel/pull/1047/commits/99535149a0297c369372666cda6895f1411707f2). Without that change in the relevant places we see the following interaction, shown on the example of User Task (Generated Form): Form Fields > Field > Properties
:
Other than that the solution feels solid, improves C7, and does not break C8 functionality (where we do not sort already :tm:). Good job!
Waiting for CI to pass before we go ahead and merge this.
This pull request aims to fix #311 . Currently, it is being developed.
Closes #311