feat: Implement add action for array items.#190
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
For the nested array/object combinations I would go with different labels following this rules:
|
| async handleAddItem({ detail }) { | ||
| const { path, itemsSchema } = detail; | ||
| this.formModel.addArrayItem(path, itemsSchema); | ||
|
|
||
| // Update the view with the new values | ||
| this.formModel = this.formModel.clone(); | ||
|
|
||
| // Persist the data | ||
| await this.formModel.saveHtml(); | ||
| } | ||
|
|
||
| async handleRemoveItem({ detail }) { | ||
| const { path } = detail; | ||
| if (!this.formModel.removeArrayItem(path)) return; | ||
|
|
||
| // Update the view with the new values | ||
| this.formModel = this.formModel.clone(); | ||
|
|
||
| // Persist the data | ||
| await this.formModel.saveHtml(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I followed the existing pattern for consistency. However, in a future PR, it would be worth refactoring this approach, as the current implementation duplicates the save logic.
There was a problem hiding this comment.
Lets keep it, could be future PR.
| case 'checkbox': inner = this.renderCheckbox(item); break; | ||
| case 'select': inner = this.renderSelect(item); break; | ||
| case 'text': inner = this.renderInput(item, 'text'); break; | ||
| case 'number': inner = this.renderInput(item, 'number'); break; | ||
| default: break; |
There was a problem hiding this comment.
In a future PR, we could extract these into separate presentational components. The editor currently appears to handle too much logic.
| * @param {string} path - The path to the array item (e.g., "data.items.[0]" or "data.items[0]") | ||
| * @returns {boolean} - True if the item was removed, false otherwise | ||
| */ | ||
| export function removeArrayItemByPath(obj, path) { |
There was a problem hiding this comment.
In the future, the utility methods responsible for patching the JSON data could be extracted into a separate utility file. This would improve readability and make the code easier to understand.
|
Tested with https://da.live/formsref?nx=issue/181#/aem-sandbox/block-collection/forms/coffee2 and it looks good to me. |
|
Make sure the lint error get fixed |
nx/blocks/form/views/editor.js
Outdated
|
|
||
| getAddItemLabel(parent) { | ||
| const itemsSchema = parent.schema?.properties?.items; | ||
| const resolved = itemsSchema && resolvePropSchema(itemsSchema, this.formModel?.schema); |
There was a problem hiding this comment.
You should not need this. You will already have the schema as part of the parent object.
There was a problem hiding this comment.
I investigated the issue and found that the annotation process was failing to annotate array items when the schema defined the item structure using the $ref/$defs mechanism.
I’ve pushed a fix to this branch. However, after refactoring the annotation process to follow a schema-first approach, this specific fix is no longer necessary. The issue is already resolved in the refactored implementation and is handled in the new PR #198.
| renderDeleteButton(item, index, isArrayItem) { | ||
| if (!isArrayItem) return nothing; | ||
| return html` | ||
| <remove-button |
There was a problem hiding this comment.
This does not need to be a Web Component.
There was a problem hiding this comment.
The form is built as a reactive UI using Lit components and the editor already manages a significant amount of logic, so to maintain clarity and separation of concerns, we should introduce well-isolated, “dumb” components. This will help keep the codebase less complex, more readable, and easier to test.
Based on my experience, as this UI continues to grow, centralizing all logic within the Editor will become increasingly difficult to maintain and, in the long term, nearly impossible to test properly.
| import { LitElement, html } from 'da-lit'; | ||
|
|
||
| const { default: getStyle } = await import('../../../../../utils/styles.js'); | ||
| const style = await getStyle(import.meta.url); |
There was a problem hiding this comment.
This file is unnecessary.
|
|
||
| if (Array.isArray(propData)) { | ||
| const resolvedItemsSchema = resolvePropSchema(key, propSchema.items, fullSchema); | ||
| const resolvedItemsSchema = resolvePropSchema(propSchema.items, fullSchema); |
There was a problem hiding this comment.
These changes should be unnecessary. I think if we have issues getting the schema recursively once, we have architectural issues we should address first.
We should read the data once, combine with the schema to create an annotated object. If we find that object does not have what we need, we need to fix the underlying issues in that first read.
I also dislike the optional chaining here as it will mask issues down the road. We want something to fail and break in these early days so we can make sure our annotating is flawless since its so complex.
There was a problem hiding this comment.
Please find my findings and comment here #190 (comment)
| this.updateHtml(); | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is good, but I also recognize I'm not sure how much logic we should put in this class.
I good guiding direction may be, "if it changes the data, it lives in the model."
There was a problem hiding this comment.
I agree, but all of these operations ultimately modify the JSON data.
From my perspective, to keep things less complex, more testable, and more readable:
- The model should represent a purely immutable state, without any HTML-related logic.
- The HTML value should be generated only once during persistence and handled at a higher level of the application.
- Updating the JSON should be implemented as a simple utility function that takes the current model, applies the necessary patches to the JSON, and then recreates the model as a new immutable state.
cc @mhaack
|
Closing this PR in favour of the PR #198, which introduce an important core update to the annotation mechanism by moving to a schema-first annotation approach. The new PR includes and adopts the changes from this PRR, which were necessary to determine and test the correct strategy during the refactoring process. |
Ticket: #181
Screen.Recording.2026-02-16.at.10.27.20.am.mov
Note: An issue has been identified in the JSON & HTML converter. It currently does not properly handle fields containing nested arrays having arrays within arrays. As a result, when the page is refreshed, the data is not loaded correctly. Will be fixed with #193
Schema examples used:
simple-test-form-inlined.schema.json
simple-test-form.schema.json