fix(editor): ensure unique ID for ReadonlyEntry to prevent state rete…#1462
fix(editor): ensure unique ID for ReadonlyEntry to prevent state rete…#1462prem22k wants to merge 2 commits intobpmn-io:developfrom
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where toggling the "Read only" property in Chrome would incorrectly retain its state when switching between different form fields. The issue was caused by the ReadonlyEntry component using a static ID, preventing proper component re-initialization when the selection changed.
Changes:
- Updated ReadonlyEntry.js to use a dynamic ID based on the field's ID (
readonly-${field.id}) - Updated test expectations in GeneralGroup.spec.js to use the new namespaced IDs
- Minor whitespace formatting changes in test helper functions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ReadonlyEntry.js | Changed from static ID 'readonly' to dynamic ID readonly-${field.id} to force component re-creation on field selection changes |
| GeneralGroup.spec.js | Updated test expectations to use 'readonly-undefined' to match the dynamic ID pattern when field.id is undefined, plus minor whitespace changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/form-js-editor/test/spec/features/properties-panel/groups/GeneralGroup.spec.js
Outdated
Show resolved
Hide resolved
|
|
||
| // then | ||
| const readonlyInput = findInput('readonly', container); | ||
| const readonlyInput = findInput('readonly-undefined', container); |
There was a problem hiding this comment.
The test fields should include an id property to better reflect real-world usage and make the tests more robust. Currently, field.id is undefined in these tests, resulting in the ID readonly-undefined. Consider adding explicit field IDs (e.g., id: 'field1') to make the tests clearer and ensure they properly validate the dynamic ID generation.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
|
||
| const { container } = renderGeneralGroup({ field, editField: editFieldSpy }); | ||
|
|
||
| const readonlyInput = findTextbox('readonly', container); | ||
| const readonlyInput = findTextbox('readonly-undefined', container); | ||
| expect(readonlyInput.textContent).to.equal('foo'); | ||
|
|
||
| // when |
There was a problem hiding this comment.
There is no test that specifically verifies the bug fix described in issue #1461. Consider adding a regression test that creates two fields with different IDs, sets readonly on the first field, switches to the second field, and verifies that the second field's readonly state is not affected. This would ensure the state retention issue doesn't reoccur.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
…oups/GeneralGroup.spec.js Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
2 participants
Closes #1461
Description:
Fixed a bug where the "Read only" property toggle would retain its state when switching between different form fields in the properties panel. This was caused by the ReadonlyEntry component reusing the same static ID
'readonly', which led to the underlying toggle component not being correctly re-initialized with the new field's value.Changes:
Updated ReadonlyEntry.js to use a dynamic ID based on the field's ID (e.g.,
readonly-${field.id}). This forces the component to be re-created when the selection changes, ensuring correct state.Updated GeneralGroup.spec.js to use the new namespaced IDs in tests to prevent regressions.
This PR adds a new
form-jselement or visually changes an existing component.