yangzhang75 commented on code in PR #8456: URL: https://github.com/apache/texera/pull/8456#discussion_r3984319753
########## frontend/src/app/common/formly/editable-label-wrapper/editable-label-wrapper.component.html: ########## @@ -0,0 +1,58 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +<!-- Authoring: the label is the input, so what you type is what the reader reads. --> +<div + class="lbl-row" + *ngIf="props.authoring"> + <input + class="lbl-input" + [value]="props.authorName" + [placeholder]="props.schemaLabel" + (change)="onRename($event)" + [attr.aria-label]="'Label shown above this input'" + [title]="'Shown above this box. Empty keeps ' + props.schemaLabel" /> + <!-- An input the reader can only lose by removing it altogether has no eye: offering + one here would be a second place to decide the same thing. --> + <button + *ngIf="props.canHide !== false" + type="button" + class="lbl-eye" + [class.off]="props.authorHidden" + (click)="onToggleHidden()" + [attr.aria-pressed]="props.authorHidden" + [attr.aria-label]="props.authorHidden ? 'Hidden from the form. Click to show' : 'Shown on the form. Click to hide'" + [title]="props.authorHidden ? 'Hidden from the form' : 'Shown on the form'"> + <i + nz-icon + [nzType]="props.authorHidden ? 'eye-invisible' : 'eye'" + nzTheme="outline"></i> + </button> +</div> + +<!-- Everyone else just reads it. --> +<label + class="lbl-static" + *ngIf="!props.authoring && (props.authorName || props.schemaLabel)"> + {{ props.authorName || props.schemaLabel }} +</label> Review Comment: Fixed in #8455: the static label now carries [attr.for]="id" (the wrapper id getter is the formly field id, which formly also puts on the control), so the reader input keeps its accessible name. Test asserts the for attribute. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.html: ########## @@ -86,7 +110,16 @@ nzType="info-circle" class="lead" aria-hidden="true"></i> - <h2>{{ instructionTitle || "How to use this" }}</h2> + <h2 *ngIf="!authoring">{{ instructionTitle || "How to use this" }}</h2> + <!-- An author edits the heading in place here, not in a separate Title box. stopPropagation + so clicking into it does not collapse the section. --> + <input + *ngIf="authoring" + class="instr-title-input" + [(ngModel)]="instructionTitle" + (ngModelChange)="onInstructionChange()" + (click)="$event.stopPropagation()" + placeholder="How to use this" /> Review Comment: Fixed in #8455. The header is a row now, not one button. Reading: one full-width toggle button with aria-expanded and aria-controls pointing at the body. Authoring: icon, the title input, and a small chevron button that toggles, so the input is a sibling of the control and not nested in it. Rendered tests cover both shapes, including input.closest("button") being null. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.html: ########## @@ -99,45 +132,149 @@ <h2>{{ instructionTitle || "How to use this" }}</h2> [hidden]="!instructionOpen"> <div class="md" + *ngIf="!authoring" [innerHTML]="instructionPreviewHtml"></div> + + <ng-container *ngIf="authoring"> + <div class="tabs"> + <button + type="button" + [attr.aria-current]="instructionMode === 'write'" + (click)="setInstructionMode('write')"> + Write + </button> + <button + type="button" + [attr.aria-current]="instructionMode === 'preview'" + (click)="setInstructionMode('preview')"> + Preview + </button> + </div> + + <ng-container *ngIf="instructionMode === 'write'"> + <textarea + class="md-input" + [(ngModel)]="instructionBody" + (ngModelChange)="onInstructionChange()" + placeholder="Explain what this does and what to fill in."></textarea> + <p class="hint">Markdown. Add a picture with <code></code>.</p> + </ng-container> + + <div + class="md" + *ngIf="instructionMode === 'preview'" + [innerHTML]="instructionPreviewHtml"></div> + </ng-container> </div> </section> <!-- The inputs an author exposed, each rendered as its operator's own field. --> <div class="pc-section-head"> <span class="label">Inputs</span> + <span + class="hint" + *ngIf="authoring" + >Drag to reorder. Click a step in the workflow to add more</span + > </div> <div class="empty" *ngIf="visibleFields.length === 0"> - This workflow has no inputs to fill in. + {{ authoring ? "No inputs yet. Open the workflow below and click a step to expose its settings." : "This workflow + has no inputs to fill in." }} </div> - <div class="params"> + <div + class="params" + cdkDropList + (cdkDropListDropped)="onDrop($event)"> <section class="card param" [class.read-only]="!canEdit" - *ngFor="let r of rendered; trackBy: trackByRendered"> - <!-- The operator's own field, so a file property gets the real file picker and an - attribute property a column dropdown. --> - <form - [formGroup]="r.form" - class="param-form"> - <formly-form - [model]="r.model" - [fields]="r.fields" - [form]="r.form"></formly-form> - </form> + *ngFor="let r of rendered; trackBy: trackByRendered" + cdkDrag + [cdkDragDisabled]="!authoring"> + <!-- Author row: the drag handle and the "From <step>" provenance line. Hidden for a broken + input, whose operatorLabel is the raw id; the broken card below says what happened. --> + <div + class="field-top" + *ngIf="authoring"> + <i + nz-icon + nzType="holder" + class="grip" + cdkDragHandle + aria-hidden="true"></i> Review Comment: Fixed in #8455: each author card has Move up / Move down buttons (aria-labelled, disabled at the ends) that go through the same by-id reorder as the drag, so a keyboard author can reorder without the handle. Unit tests for the move and the end stops, rendered test for the buttons. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -446,14 +526,69 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } + private operators(): OperatorPredicate[] { + return this.workflowActionService.getTexeraGraph().getAllOperators(); + } + + /** + * Drop exposed inputs whose operator was deleted: they can never be filled, and a re-added + * operator gets a fresh id so they could not reconnect. Guarded to edit mode and after load, so a + * reader never mutates the workflow and a not-yet-seeded mid-load graph never deletes a still-valid + * input. Only the operator-gone case, not a transiently missing property schema. + */ + private pruneBrokenBindings(): void { + if (this.loading || !this.authoring) { + return; + } + const graph = this.workflowActionService.getTexeraGraph(); + const configFields = this.formBindingService.getConfig().fields; + const alive = configFields.filter(p => graph.hasOperator(p.operatorID)); + if (alive.length !== configFields.length) { + this.formBindingService.setFields(alive); Review Comment: You were right, and it was a contradiction inside the PR: readConfig pruned before resolveFields ran, so the broken card path (visibleFields for authors, the empty card with the reason, the remove button) was unreachable. pruneBrokenBindings is removed in #8455. A broken input now stays until the author removes it explicitly, readers never see it, and re-reading the config no longer writes it. Test replaced accordingly. ########## frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts: ########## @@ -631,10 +636,19 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On this.currentOperatorSchema = this.dynamicSchemaService.getDynamicSchema(this.currentOperatorId); this.currentOperatorStatus = this.workflowStatusSerivce.getCurrentStatus()[this.currentOperatorId]; - this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("currentlyEditing", this.currentOperatorId); + if (this.broadcastEditing) { + this.workflowActionService + .getTexeraGraph() + .updateSharedModelAwareness("currentlyEditing", this.currentOperatorId); + } const operator = this.workflowActionService.getTexeraGraph().getOperator(this.currentOperatorId); - // set the operator data needed - this.workflowActionService.setOperatorVersion(operator.operatorID, this.currentOperatorSchema.operatorVersion); + // Syncing the operator to the current schema version writes the new version into the Yjs shared + // model (changeOperatorVersion), which broadcasts and persists. That is right on the canvas, but + // a read-only inspect (broadcastEditing=false) must not mutate the workflow just by opening a + // step, so skip the sync there and show the version as stored. + if (this.broadcastEditing) { + this.workflowActionService.setOperatorVersion(operator.operatorID, this.currentOperatorSchema.operatorVersion); Review Comment: Fixed upstream in #8442 (this PR sits on it): every write the frame can make is gated on one input, renamed actsAsEditor, including the ajv-defaults form-change path and the ui-parameter sync, with a fake-timer test asserting nothing is written on open. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
