-
Notifications
You must be signed in to change notification settings - Fork 207
Revert keeping selected element in sync with canvas state #1165
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
Conversation
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.
Pull Request Overview
This PR refactors the properties panel rendering logic by moving event handling and state management from the BpmnPropertiesPanelRenderer class into the BpmnPropertiesPanel React component. This enables better React lifecycle management and simplifies the renderer's responsibilities.
- Event listeners moved from renderer to React component using useEffect hooks
- State management now handled via React hooks (useState, useMemo, useCallback)
- Renderer simplified to only handle root.added events and delegate to the React component
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/render/BpmnPropertiesPanelRenderer.js | Removed state management and event handling logic; simplified to only handle root.added event |
| src/render/BpmnPropertiesPanel.js | Added React hooks for event handling, state management, and layout changes; moved logic from renderer |
| test/spec/BpmnPropertiesPanel.spec.js | Updated tests to reflect new component-based event handling; added tests for new event behaviors |
| test/spec/BpmnPropertiesPanelRenderer.spec.js | Removed redundant rendering tests; simplified to test root.added behavior |
| src/provider/bpmn/BpmnPropertiesProvider.js | Reordered constructor statements to register provider before storing injector |
| src/provider/zeebe/ZeebePropertiesProvider.js | Reordered constructor statements to register provider before storing injector |
| src/provider/camunda-platform/CamundaPlatformPropertiesProvider.js | Reordered constructor statements to register provider before storing injector |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@philippfromme any root cause analysis for the problem? |
Unfortunately I haven't found the exact root cause yet. But I suspect that part of the problem lies in the core properties panel (cf. https://github.com/bpmn-io/properties-panel/blob/51649e1450acb06a566435d14f75339bdaf5af59/src/PropertiesPanel.js#L142) and that it was more or less accidentally working. I need to look into this again. |
Proposed Changes
Due to the numerous issues found (camunda/camunda-modeler#5386) this reverts the changes that lead to these issues. We'll have to look into this again and maybe think about better ways to test the properties panel to make sure we catch these issues in the future.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}