refactor(ui): redesign the shared key/value field across its three consumers (#37191) - #37284
refactor(ui): redesign the shared key/value field across its three consumers (#37191)#37284adrianjm-dotCMS wants to merge 6 commits into
Conversation
…nsumers (#37191) Rework the one shared Key/Value editor in `libs/ui` to the new design and roll it out to all three of its consumers: the Edit Content field, the Content Type Field Variables tab, and the Apps custom-properties panel. The redesign is assembled from stock PrimeNG 21 — no new component enters the workspace, and the net component count for the feature is zero. What changed for the user: - Values render as plain text at rest and become an input when activated; Enter commits, Escape restores. - Row actions (drag handle, remove) are revealed on hover instead of competing for attention on every row. - Hidden values move from a toggle switch in its own column to an eye inside the value field. It is a button in both states, so hiding a value is reversible. - The hidden indicator is never hover-gated: which values are secret has to be readable at a glance. - Empty state gains an icon and reuses the existing `keyValue.value_no_rows.label` copy, which already matched the design. - Reordering is offered in all three consumers rather than only one. Implementation notes worth knowing: - The row components attach by attribute selector (`tr[dotKeyValueTableRow]`), so their host IS the `tr`. PrimeNG themes its table with direct-child combinators (`.p-datatable-tbody > tr > td`), and an element wrapper between the tbody and the tr silently defeats every table style the theme provides. This is why the previous markup rendered with no borders or header band. - `onRowReorder` must not re-apply the move: PrimeNG's `onRowDrop` already calls `reorderArray` on the array bound to `[value]`. The handler only re-publishes it with a fresh reference so the signal notifies. - Hover-hidden actions use `opacity-0`, never `display:none` or an `@if` — those remove the control from the tab order and strand keyboard and touch users. A test asserts the mechanism, not merely that the icon is invisible. Also removed along the way: `DotKeyValueUtil` (87 lines nothing imported), a stale `[autoFocus]` binding in the Apps template together with the spec stub that made it look real, a duplicated copy of the row template that tests were asserting against instead of the real one, the now-unused `dragAndDrop` input, and both SCSS files. Icons follow the repo standard (Material Symbols); PrimeIcons are gone from this component's authored markup. No new i18n key was needed. Tests: 61 in `libs/ui` (from 31), 24 in `edit-content`, 70 in `dotcms-ui`, plus Playwright smokes for each consumer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
Area : Frontend
|
Claude finished @adrianjm-dotCMS's task in 6m 44s —— View job PR Review — Key/Value field redesignReviewed the full diff against the merge base. The refactor is well-constructed: the attribute-selector A few things worth a look before merge — none are provable defects, so all Medium: New Issues
Notes (non-blocking)
The util-level tests ( |
…coverage This update refines the key/value field component across various consumers, ensuring consistent behavior and improved user experience. Key changes include: - Implemented a new method for closing the field dialog using the Escape key, enhancing accessibility. - Updated tests to utilize unique keys for entries, preventing collisions and ensuring reliable test outcomes. - Enhanced the visibility control for hidden values, making it clearer when values are masked. - Introduced pagination for long lists in the key/value component, allowing users to load more entries as needed. These changes aim to streamline interactions and maintain data integrity across the application.
erickgonzalez
left a comment
erickgonzalez
left a comment
There was a problem hiding this comment.
Spec looks good! Let's continue with the implementation.
Recovering a Key/Value field's key order picked its target by shape — "a plain object whose values are all primitives". A binary field's `metaData` is exactly that, so it was rewritten into JSON text, `contentlet.metaData.name` stopped being readable, and the file preview came back empty after a reload (`binary-field.spec.ts`, CI). `getContentById` serves every field type, so a guess there reaches all five of its callers. The decision moves to the one place that knows a field's type: the KEY_VALUE entry in the form's resolution map. `getContentById` now hands back the contentlet untouched and parks the order-preserving parse alongside it, under a key of its own; the resolver reads its own variable out of that and nothing else can be mistaken for it. Also fixes two E2E specs that only passed on a machine seeded a particular way: - Apps custom properties drove SSO — SAML, which requires nine parameters including certificates. Save is bound to the whole form's validity, so it is disabled on a fresh instance. Now uses an app that requires one string, filled by the spec, and waits for the navigation a save triggers instead of racing it. - The field dialog was closed with Escape, which depends on where focus is. Uses the dialog's own close button, which is always present.
The Field Variables persistence check closed the dialog to reopen it. In CI the close button was found, visible and stable, but every click was intercepted by a modal mask sitting above it, so the test spent its whole timeout retrying. Reloading the builder gets at what the test is actually for — that each row was written against the field rather than held in the dialog's state — and does it more strictly, since nothing at all survives a reload. It also has no modal in its way. `closeFieldDialog` had no other caller and could not be relied on, so it goes rather than sitting there for someone to reach for.
The previous commit meant to reload but called `goToBuilder`, which navigates to the URL already in the address bar. That is a same-document fragment navigation: nothing is torn down, so the field dialog stayed open and its header intercepted the click meant for the field underneath. `reloadBuilder` does a real reload and waits for the builder to come back. With the local instance up again this reproduced the CI failure exactly, and the four specs now pass twice over.
The Field Variables persistence check reloaded straight after deleting a row. On screen the row was already gone — the shared editor drops it from its own list on click, while the consumer only commits once the request returns — so `expectKeyAbsent` passed while the DELETE was still in flight. The reload overtook it and the row came back, failing in CI and passing here. Each mutation now waits for the response from the per-row endpoint and asserts it succeeded, so the reload can only happen after the write has landed. Verified by holding that endpoint for 3s locally, which reproduces CI: without the wait the row is back after the reload, with it the delete survives.
2 participants
Fixes #37191
Proposed Changes
Reworks the one shared Key/Value editor in
libs/uito the new design and rolls it out to all threeof its consumers — the Edit Content field, the Content Type Field Variables tab, and the
Apps custom-properties panel — so they finally look and behave the same.
Built from stock PrimeNG 21. No new component enters the workspace; the net component count for
this feature is zero.
What changed for the user
Three findings worth a reviewer's attention
.p-datatable-tbody > tr > td). The row components used element selectors withdisplay: contents, so the component element sat between thetbodyand thetrand no table style from the theme ever applied — no borders, no header band. They now attach by attribute selector (tr[dotKeyValueTableRow]), making the host thetritself.onRowReordermust not re-apply the move. PrimeNG'sonRowDropalready callsreorderArrayon the array bound to[value]. Recomputing fromdragIndex/dropIndexapplies it twice. The handler only re-publishes the array with a fresh reference so the signal notifies.opacity-0, neverdisplay:noneor an@if. Those remove the control from the tab order and strand keyboard and touch users with no visible symptom for anyone on a mouse. A test asserts the mechanism, not merely that the icon is invisible.Removed along the way
DotKeyValueUtil— 87 lines nothing imported.[autoFocus]binding in the Apps template, plus the spec stub that made it look real. It compiled only becausestrictTemplatesis off in that app.dragAndDropinput, once reordering became universal and it had nothing left to gate.Checklist
keyValue.value_no_rows.labelAdditional Info
Tests: 61 in
libs/ui(from 31), 24 inedit-content, 70 indotcms-ui, plus Playwright smokes for each consumer. Lint and build clean.Not yet run: the Playwright specs have a clean typecheck but have not been executed, and the manual keyboard-only pass is still pending. Flagging it rather than implying green.
Scope: frontend only. Order persistence for Field Variables and Apps was scoped out of this issue — it would mean database, REST and encrypted-storage work.
spec.md,data-model.mdandcontracts/are included as the reviewed contract; the Spec-Kit process artifacts are gitignored by design.Screenshots
🤖 Generated with Claude Code