-
Notifications
You must be signed in to change notification settings - Fork 23
feat: Implement add action for array items. #190
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
Changes from 10 commits
12beeba
3976f26
77c647a
6e6f6cc
d89e16c
71e41a4
8540b28
219e23b
6557a17
4446ca6
95b6ef8
755a263
efe6b5c
88a028f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,6 +80,28 @@ class FormEditor extends LitElement { | |
| await this.formModel.saveHtml(); | ||
| } | ||
|
|
||
| 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(); | ||
| } | ||
|
|
||
|
Comment on lines
+83
to
+104
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets keep it, could be future PR. |
||
| renderSchemaSelector() { | ||
| return html` | ||
| <p class="da-form-title">Please select a schema to get started</p> | ||
|
|
@@ -103,7 +125,12 @@ class FormEditor extends LitElement { | |
|
|
||
| return html` | ||
| <div class="da-form-editor"> | ||
| <da-form-editor @update=${this.handleUpdate} .formModel=${this.formModel}></da-form-editor> | ||
| <da-form-editor | ||
| @update=${this.handleUpdate} | ||
| @add-item=${this.handleAddItem} | ||
| @remove-item=${this.handleRemoveItem} | ||
| .formModel=${this.formModel} | ||
| ></da-form-editor> | ||
| <da-form-preview .formModel=${this.formModel}></da-form-preview> | ||
| </div>`; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,81 @@ | ||
| import { daFetch } from 'https://da.live/blocks/shared/utils.js'; | ||
|
|
||
| function parsePath(path) { | ||
| const parts = path.split('.').flatMap((part) => { | ||
| // Handle consecutive indices like "[0][0]" first (before arrayMatch grabs "[0]" as key) | ||
| if (part.startsWith('[')) { | ||
| const indices = part.match(/\[(\d+)\]/g); | ||
| if (indices) return indices.map((i) => parseInt(i.slice(1, -1), 10)); | ||
| } | ||
| const arrayMatch = part.match(/^(.+?)\[(\d+)\]$/); | ||
| if (arrayMatch) return [arrayMatch[1], parseInt(arrayMatch[2], 10)]; | ||
| const indexMatch = part.match(/^\[(\d+)\]$/); | ||
| if (indexMatch) return [parseInt(indexMatch[1], 10)]; | ||
| return part; | ||
| }); | ||
| return parts; | ||
| } | ||
|
|
||
| export async function loadHtml(details) { | ||
| const resp = await daFetch(details.sourceUrl); | ||
| if (!resp.ok) return { error: 'Could not fetch doc' }; | ||
| return { html: (await resp.text()) }; | ||
| } | ||
|
|
||
| /** | ||
| * Gets a value from an object using a path string. | ||
| * @param {Object} obj - The object to read from | ||
| * @param {string} path - The path string (e.g. "data.items[0].name") | ||
| * @returns {*} - The value at the path, or undefined | ||
| */ | ||
| export function getValueByPath(obj, path) { | ||
| const parts = parsePath(path); | ||
| let current = obj; | ||
| for (const part of parts) { | ||
| if (current == null) return undefined; | ||
| current = current[part]; | ||
| } | ||
| return current; | ||
| } | ||
|
|
||
| /** | ||
| * Removes an array item at the given path by splicing it from its parent array. | ||
| * @param {Object} obj - The object to modify | ||
| * @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) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| const parts = parsePath(path); | ||
| if (parts.length < 2) return false; | ||
|
|
||
| // Path must end with an array index (e.g. "data.items.[0]" → index 0) | ||
| const lastPart = parts[parts.length - 1]; | ||
| if (typeof lastPart !== 'number') return false; | ||
|
|
||
| // Navigate to the parent of the target array | ||
| const parentParts = parts.slice(0, -1); | ||
| let current = obj; | ||
| for (let i = 0; i < parentParts.length - 1; i += 1) { | ||
| const part = parentParts[i]; | ||
| if (current == null || !(part in current)) return false; | ||
| // Step into each segment (e.g. obj → obj.data → obj.data.items) | ||
| current = current[part]; | ||
| } | ||
|
|
||
| // Key of the array (e.g. "items") | ||
| const parentKey = parentParts[parentParts.length - 1]; | ||
| if (current == null || !(parentKey in current) | ||
| || !Array.isArray(current[parentKey])) return false; | ||
|
|
||
| const array = current[parentKey]; | ||
| const index = lastPart; | ||
| if (index < 0 || index >= array.length) return false; | ||
|
|
||
| // Remove the item in-place | ||
| array.splice(index, 1); | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Sets a value on an object using a path string. | ||
| * Supports dot notation and array indices. | ||
|
|
@@ -18,20 +88,7 @@ export async function loadHtml(details) { | |
| * // obj.data.items[0].name is now 'updated' | ||
| */ | ||
| export function setValueByPath(obj, path, value) { | ||
| // Split the path into parts, handling both dots and brackets | ||
| const parts = path.split('.').flatMap((part) => { | ||
| // Handle array indices like "items[0]" -> ["items", "0"] | ||
| const arrayMatch = part.match(/^(.+?)\[(\d+)\]$/); | ||
| if (arrayMatch) { | ||
| return [arrayMatch[1], parseInt(arrayMatch[2], 10)]; | ||
| } | ||
| // Handle standalone array index like "[0]" | ||
| const indexMatch = part.match(/^\[(\d+)\]$/); | ||
| if (indexMatch) { | ||
| return [parseInt(indexMatch[1], 10)]; | ||
| } | ||
| return part; | ||
| }); | ||
| const parts = parsePath(path); | ||
|
|
||
| // Navigate to the parent of the final property | ||
| let current = obj; | ||
|
|
@@ -49,17 +106,17 @@ export function setValueByPath(obj, path, value) { | |
| current[parts[parts.length - 1]] = value; | ||
| } | ||
|
|
||
| function resolvePropSchema(key, localSchema, fullSchema) { | ||
| export function resolvePropSchema(localSchema, fullSchema) { | ||
| const { title } = localSchema; | ||
|
|
||
| if (localSchema.$ref) { | ||
| const path = localSchema.$ref.substring(2).split('/')[1]; | ||
|
|
||
| // try local ref | ||
| let def = localSchema.$defs?.[path]; | ||
| let def = localSchema?.$defs?.[path]; | ||
| // TODO: walk up the tree looking for the def | ||
| // try global ref | ||
| if (!def) def = fullSchema.$defs?.[path]; | ||
| if (!def) def = fullSchema?.$defs?.[path]; | ||
| if (def) { | ||
| if (!title) return def; | ||
| return { ...def, title }; | ||
|
|
@@ -82,10 +139,10 @@ export function annotateProp(key, propData, propSchema, fullSchema, path = '', r | |
| const currentPath = path ? `${path}.${key}` : key; | ||
|
|
||
| // Will have schema.props | ||
| const resolvedSchema = resolvePropSchema(key, propSchema, fullSchema); | ||
| const resolvedSchema = resolvePropSchema(propSchema, fullSchema); | ||
|
|
||
| if (Array.isArray(propData)) { | ||
| const resolvedItemsSchema = resolvePropSchema(key, propSchema.items, fullSchema); | ||
| const resolvedItemsSchema = resolvePropSchema(propSchema.items, fullSchema); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please find my findings and comment here #190 (comment) |
||
|
|
||
| // It's possible that items do not have a title, let them inherit from the parent | ||
| resolvedItemsSchema.title ??= resolvedSchema.title; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| :host { | ||
| display: inline-block; | ||
| } | ||
|
|
||
| .remove-btn { | ||
| flex-shrink: 0; | ||
| width: 28px; | ||
| height: 28px; | ||
| padding: 0; | ||
| margin: 0; | ||
| background: transparent; | ||
| color: var(--s2-gray-700, #374151); | ||
| border: 1px solid transparent; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| transition: background 0.15s ease, color 0.15s ease; | ||
|
|
||
| &:hover { | ||
| background: var(--s2-gray-200, #e5e7eb); | ||
| } | ||
|
|
||
| &.confirm-state { | ||
| background: var(--s2-red-600, #dc2626); | ||
| color: white; | ||
| border: none; | ||
|
|
||
| &:hover { | ||
| background: var(--s2-red-700, #b91c1c); | ||
| color: white; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .trash-icon, | ||
| .check-icon { | ||
| width: 16px; | ||
| height: 16px; | ||
| flex-shrink: 0; | ||
| } | ||
|
|
||
| .check-icon { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| font-size: 14px; | ||
| font-weight: 600; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import { LitElement, html } from 'da-lit'; | ||
|
|
||
| const { default: getStyle } = await import('../../../../../utils/styles.js'); | ||
| const style = await getStyle(import.meta.url); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is unnecessary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This issue is linked to #190 (comment) |
||
|
|
||
| /** | ||
| * A two-stage confirmation remove button. | ||
| * First click: Shows checkmark (confirmation state) | ||
| * Second click: Dispatches 'remove-item' event | ||
| * Auto-reverts to trash icon after 3 seconds if not confirmed | ||
| */ | ||
| class RemoveButton extends LitElement { | ||
| static properties = { | ||
| path: { type: String }, | ||
| index: { type: Number }, | ||
| confirmState: { state: true }, | ||
| }; | ||
|
|
||
| constructor() { | ||
| super(); | ||
| this.path = ''; | ||
| this.index = null; | ||
| this.confirmState = false; | ||
| this.timeoutId = null; | ||
| } | ||
|
|
||
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| this.shadowRoot.adoptedStyleSheets = [style]; | ||
| } | ||
|
|
||
| disconnectedCallback() { | ||
| this.clearConfirmTimeout(); | ||
| super.disconnectedCallback(); | ||
| } | ||
|
|
||
| clearConfirmTimeout() { | ||
| if (this.timeoutId) { | ||
| clearTimeout(this.timeoutId); | ||
| this.timeoutId = null; | ||
| } | ||
| } | ||
|
|
||
| handleClick(e) { | ||
| e.stopPropagation(); | ||
|
|
||
| if (this.confirmState) { | ||
| this.clearConfirmTimeout(); | ||
| this.confirmState = false; | ||
| this.dispatchEvent(new CustomEvent('remove-item', { | ||
| detail: { path: this.path }, | ||
| bubbles: true, | ||
| composed: true, | ||
| })); | ||
| return; | ||
| } | ||
|
|
||
| this.confirmState = true; | ||
| this.timeoutId = setTimeout(() => { | ||
| this.confirmState = false; | ||
| this.timeoutId = null; | ||
| }, 3000); | ||
| } | ||
|
|
||
| render() { | ||
| const indexLabel = this.index != null ? `Remove item ${this.index}` : 'Remove item'; | ||
| const ariaLabel = this.confirmState ? 'Confirm removal' : indexLabel; | ||
| const title = this.confirmState | ||
| ? 'Click to confirm removal' | ||
| : 'Remove this item'; | ||
|
|
||
| return html` | ||
| <button | ||
| class="remove-btn ${this.confirmState ? 'confirm-state' : ''}" | ||
| @click=${this.handleClick} | ||
| title="${title}" | ||
| aria-label="${ariaLabel}" | ||
| > | ||
| ${this.confirmState | ||
| ? html`<span class="check-icon">✓</span>` | ||
| : html` | ||
| <svg class="trash-icon" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"> | ||
| <polyline points="3 6 5 6 21 6"/> | ||
| <path d="M19 6l-1 14a2 2 0 0 1-2 2H8a2 2 0 0 1-2-2L5 6"/> | ||
| <path d="M10 11v6"/> | ||
| <path d="M14 11v6"/> | ||
| <path d="M9 6V4a2 2 0 0 1 2-2h2a2 2 0 0 1 2 2v2"/> | ||
| </svg> | ||
| `} | ||
| </button> | ||
| `; | ||
| } | ||
| } | ||
|
|
||
| customElements.define('remove-button', RemoveButton); | ||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
cc @mhaack