From 32bb89235e18a5c52287b7decf98c554be088b0e Mon Sep 17 00:00:00 2001 From: George Gevoian Date: Thu, 18 Nov 2021 21:35:01 -0800 Subject: [PATCH] (core) Polish Importer UI Summary: Changes include: * Hide the colum matching section for new destinations (for now). * Make the preview table read-only. * Don't show helper column IDs when the formula editor is open. * Fix the formula editor autocomplete to show suggestions from the active transform section. * Hide the formula icons in the preview table, and other unnecessary UI elements such as row dropdown menus. * Keep preview loading spinner shown if scheduled (i.e. debounced) diff updates exist. Test Plan: Browser tests. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3148 --- app/client/components/BaseView.js | 7 +- app/client/components/GridView.js | 12 +- app/client/components/Importer.ts | 178 +++++++++++++++++------------ app/client/widgets/FieldBuilder.ts | 11 +- 4 files changed, 121 insertions(+), 87 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 40d263db..f7459c78 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -30,6 +30,7 @@ const {encodeObject} = require("app/plugin/objtypes"); /** * BaseView forms the basis for ViewSection classes. * @param {Object} viewSectionModel - The model for the viewSection represented. + * @param {Boolean} options.isPreview - Whether the view is a read-only preview (e.g. Importer view). * @param {Boolean} options.addNewRow - Whether to include an add row in the model. */ function BaseView(gristDoc, viewSectionModel, options) { @@ -168,6 +169,8 @@ function BaseView(gristDoc, viewSectionModel, options) { return linking && linking.disableEditing(); })); + this.isPreview = this.options.isPreview; + this.enableAddRow = this.autoDispose(ko.computed(() => this.options.addNewRow && !this.viewSection.disableAddRemoveRows() && !this.disableEditing())); @@ -198,7 +201,9 @@ function BaseView(gristDoc, viewSectionModel, options) { // A koArray of FieldBuilder objects, one for each view-section field. this.fieldBuilders = this.autoDispose( - FieldBuilder.createAllFieldWidgets(this.gristDoc, this.viewSection.viewFields, this.cursor) + FieldBuilder.createAllFieldWidgets(this.gristDoc, this.viewSection.viewFields, this.cursor, { + isPreview: this.isPreview, + }) ); // An observable evaluating to the FieldBuilder for the field where the cursor is. diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index 5430ca51..d5968be0 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -55,7 +55,7 @@ const ROW_NUMBER_WIDTH = 52; * GridView component implements the view of a grid of cells. */ function GridView(gristDoc, viewSectionModel, isPreview = false) { - BaseView.call(this, gristDoc, viewSectionModel, { 'addNewRow': true }); + BaseView.call(this, gristDoc, viewSectionModel, { isPreview, 'addNewRow': true }); this.viewSection = viewSectionModel; @@ -878,7 +878,7 @@ GridView.prototype.buildDom = function() { kd.style('borderLeftWidth', v.borderWidthPx), kd.foreach(v.viewFields(), field => { var isEditingLabel = ko.pureComputed({ - read: () => this.gristDoc.isReadonlyKo() ? false : editIndex() === field._index(), + read: () => this.gristDoc.isReadonlyKo() || self.isPreview ? false : editIndex() === field._index(), write: val => editIndex(val ? field._index() : -1) }).extend({ rateLimit: 0 }); let filterTriggerCtl; @@ -899,10 +899,10 @@ GridView.prototype.buildDom = function() { if (btn) { btn.click(); } }), dom('div.g-column-label', - kf.editableLabel(field.displayLabel, isEditingLabel, renameCommands), + kf.editableLabel(self.isPreview ? field.label : field.displayLabel, isEditingLabel, renameCommands), dom.on('mousedown', ev => isEditingLabel() ? ev.stopPropagation() : true) ), - this.isPreview ? null : menuToggle(null, + self.isPreview ? null : menuToggle(null, kd.cssClass('g-column-main-menu'), kd.cssClass('g-column-menu-btn'), // Prevent mousedown on the dropdown triangle from initiating column drag. @@ -1001,7 +1001,7 @@ GridView.prototype.buildDom = function() { ev.preventDefault(); ev.currentTarget.querySelector('.menu_toggle').click(); }), - menuToggle(null, + self.isPreview ? null : menuToggle(null, dom.on('click', ev => self.maybeSelectRow(ev.currentTarget.parentNode, row.getRowId())), menu(() => RowContextMenu({ disableInsert: Boolean(self.gristDoc.isReadonly.get() || self.viewSection.disableAddRemoveRows() || self.tableModel.tableMetaRow.onDemand()), @@ -1378,7 +1378,7 @@ GridView.prototype._getColumnMenuOptions = function(copySelection) { numColumns: copySelection.fields.length, numFrozen: this.viewSection.numFrozen.peek(), disableModify: calcFieldsCondition(copySelection.fields, f => f.disableModify.peek()), - isReadonly: this.gristDoc.isReadonly.get(), + isReadonly: this.gristDoc.isReadonly.get() || this.isPreview, isFiltered: this.isFiltered(), isFormula: calcFieldsCondition(copySelection.fields, f => f.column.peek().isRealFormula.peek()), }; diff --git a/app/client/components/Importer.ts b/app/client/components/Importer.ts index 6305b652..82a7867c 100644 --- a/app/client/components/Importer.ts +++ b/app/client/components/Importer.ts @@ -170,7 +170,16 @@ export class Importer extends DisposableWithEvents { // Promise for the most recent generateImportDiff action. private _lastGenImportDiffPromise: Promise|null = null; - private _updateImportDiff = debounce(this._updateDiff, 1000, {leading: true, trailing: true}); + private _debouncedUpdateDiff = debounce(this._updateDiff, 1000, {leading: true, trailing: true}); + + /** + * Flag that is set when _updateImportDiff is called, and unset when _debouncedUpdateDiff begins executing. + * + * This is a workaround until Lodash's next release, which supports checking if a debounced function is + * pending. We need to know if more debounced calls are pending so that we can decide to take down the + * loading spinner over the preview table, or leave it up until all scheduled calls settle. + */ + private _hasScheduledDiffUpdate = false; // destTables is a list of options for import destinations, and includes all tables in the // document, plus two values: to import as a new table, and to skip an import table entirely. @@ -297,6 +306,9 @@ export class Importer extends DisposableWithEvents { // Otherwise, update the transform section for `sourceInfo`. sourceInfo.transformSection.set(this._gristDoc.docModel.viewSections.getRowModel(transformSectionRef)); sourceInfo.isLoadingSection.set(false); + + // Change the active section to the transform section, so that formula autocomplete works. + this._gristDoc.viewModel.activeSectionId(transformSectionRef); } private _getTransformedDataSource(upload: UploadResult): DataSourceTransformed { @@ -479,16 +491,28 @@ export class Importer extends DisposableWithEvents { * Triggers an update of the import diff in the preview table. When called in quick succession, * only the most recent call will result in an update being made to the preview table. * - * NOTE: This method should not be called directly. Instead, use _updateImportDiff, which - * wraps this method and debounces it. + * @param {SourceInfo} info The source to update the diff for. + */ + private async _updateImportDiff(info: SourceInfo) { + this._hasScheduledDiffUpdate = true; + this._isLoadingDiff.set(true); + await this._debouncedUpdateDiff(info); + } + + /** + * NOTE: This method should not be called directly. Instead, use _updateImportDiff above, which + * wraps this method and calls a debounced version of it. + * + * Triggers an update of the import diff in the preview table. When called in quick succession, + * only the most recent call will result in an update being made to the preview table. * * @param {SourceInfo} info The source to update the diff for. */ private async _updateDiff(info: SourceInfo) { + // Reset the flag tracking scheduled updates since the debounced update has started. + this._hasScheduledDiffUpdate = false; + const mergeOptions = this._mergeOptions[info.hiddenTableId]!; - - this._isLoadingDiff.set(true); - if (!mergeOptions.updateExistingRecords.get() || mergeOptions.mergeCols.get().length === 0) { // We can simply disable document comparison mode when merging isn't configured. this._gristDoc.comparison = null; @@ -506,7 +530,10 @@ export class Importer extends DisposableWithEvents { this._gristDoc.comparison = diff; } - this._isLoadingDiff.set(false); + // If more updates where scheduled since we started the update, leave the loading spinner up. + if (!this._hasScheduledDiffUpdate) { + this._isLoadingDiff.set(false); + } } /** @@ -523,8 +550,9 @@ export class Importer extends DisposableWithEvents { * is functionally equivalent to canceling the outstanding requests. */ private _cancelPendingDiffRequests() { - this._updateImportDiff.cancel(); + this._debouncedUpdateDiff.cancel(); this._lastGenImportDiffPromise = null; + this._hasScheduledDiffUpdate = false; this._isLoadingDiff.set(false); } @@ -579,14 +607,14 @@ export class Importer extends DisposableWithEvents { const {mergeCols, updateExistingRecords, hasInvalidMergeCols} = this._mergeOptions[info.hiddenTableId]!; return cssConfigAndPreview( - cssConfigColumn( - dom.maybe(info.transformSection, section => [ - dom.maybe(info.destTableId, () => { - const updateRecordsListener = updateExistingRecords.addListener(async () => { - await this._updateImportDiff(info); - }); + dom.maybe(info.destTableId, () => cssConfigColumn( + dom.maybe(info.transformSection, section => { + const updateRecordsListener = updateExistingRecords.addListener(async () => { + await this._updateImportDiff(info); + }); - return cssMergeOptions( + return [ + cssMergeOptions( cssMergeOptionsToggle(labeledSquareCheckbox( updateExistingRecords, 'Update existing records', @@ -619,77 +647,77 @@ export class Importer extends DisposableWithEvents { ) ]; }) - ); - }), - dom.domComputed(this._unmatchedFields, fields => - fields && fields.length > 0 ? - cssUnmatchedFields( - dom('div', - cssGreenText( - `${fields.length} unmatched ${fields.length > 1 ? 'fields' : 'field'}` + ), + dom.domComputed(this._unmatchedFields, fields => + fields && fields.length > 0 ? + cssUnmatchedFields( + dom('div', + cssGreenText( + `${fields.length} unmatched ${fields.length > 1 ? 'fields' : 'field'}` + ), + ' in import:' ), - ' in import:' - ), - cssUnmatchedFieldsList(fields.join(', ')), - testId('importer-unmatched-fields') - ) : null - ), - cssColumnMatchOptions( - dom.forEach(fromKo(section.viewFields().getObservable()), field => cssColumnMatchRow( - cssColumnMatchIcon('ImportArrow'), - cssSourceAndDestination( - cssDestinationFieldRow( - cssDestinationFieldLabel( - dom.text(field.label), - ), - cssDestinationFieldSettings( - icon('Dots'), - menu( - () => { - const sourceColId = field.origCol().id(); - const sourceColIdsAndLabels = [...this._sourceColLabelsById.get()!.entries()]; - return [ - menuItem( - () => this._gristDoc.clearColumns([sourceColId]), - 'Skip', - testId('importer-column-match-menu-item') - ), - menuDivider(), - ...sourceColIdsAndLabels.map(([id, label]) => + cssUnmatchedFieldsList(fields.join(', ')), + testId('importer-unmatched-fields') + ) : null + ), + cssColumnMatchOptions( + dom.forEach(fromKo(section.viewFields().getObservable()), field => cssColumnMatchRow( + cssColumnMatchIcon('ImportArrow'), + cssSourceAndDestination( + cssDestinationFieldRow( + cssDestinationFieldLabel( + dom.text(field.label), + ), + cssDestinationFieldSettings( + icon('Dots'), + menu( + () => { + const sourceColId = field.origCol().id(); + const sourceColIdsAndLabels = [...this._sourceColLabelsById.get()!.entries()]; + return [ menuItem( - () => this._setColumnFormula(sourceColId, '$' + id), - label, + () => this._gristDoc.clearColumns([sourceColId]), + 'Skip', testId('importer-column-match-menu-item') ), - ), - testId('importer-column-match-menu'), - ]; - }, - {placement: 'right-start'}, + menuDivider(), + ...sourceColIdsAndLabels.map(([id, label]) => + menuItem( + () => this._setColumnFormula(sourceColId, '$' + id), + label, + testId('importer-column-match-menu-item') + ), + ), + testId('importer-column-match-menu'), + ]; + }, + { placement: 'right-start' }, + ), + testId('importer-column-match-destination-settings') ), - testId('importer-column-match-destination-settings') + testId('importer-column-match-destination') ), - testId('importer-column-match-destination') - ), - dom.domComputed(use => dom.create( - this._buildColMappingFormula.bind(this), - use(field.column), - (elem: Element) => this._activateFormulaEditor(elem, field), - 'Skip' - )), - testId('importer-column-match-source-destination'), - ) - )), - testId('importer-column-match-options'), - ) - ]), - ), + dom.domComputed(use => dom.create( + this._buildColMappingFormula.bind(this), + use(field.column), + (elem: Element) => this._activateFormulaEditor(elem, field), + 'Skip' + )), + testId('importer-column-match-source-destination'), + ) + )), + testId('importer-column-match-options'), + ) + ]; + }), + )), cssPreviewColumn( cssSectionHeader('Preview'), dom.domComputed(use => { const previewSection = use(this._previewViewSection); if (use(this._isLoadingDiff) || !previewSection) { - return cssPreviewSpinner(loadingSpinner()); + return cssPreviewSpinner(loadingSpinner(), testId('importer-preview-spinner')); } const gridView = this._createPreview(previewSection); diff --git a/app/client/widgets/FieldBuilder.ts b/app/client/widgets/FieldBuilder.ts index 0ca38496..6525fe3c 100644 --- a/app/client/widgets/FieldBuilder.ts +++ b/app/client/widgets/FieldBuilder.ts @@ -36,10 +36,10 @@ const testId = makeTestId('test-fbuilder-'); // Creates a FieldBuilder object for each field in viewFields export function createAllFieldWidgets(gristDoc: GristDoc, viewFields: ko.Computed>, - cursor: Cursor) { + cursor: Cursor, options: { isPreview?: boolean } = {}) { // TODO: Handle disposal from the map when fields are removed. return viewFields().map(function(field) { - return new FieldBuilder(gristDoc, field, cursor); + return new FieldBuilder(gristDoc, field, cursor, options); }).setAutoDisposeValues(); } @@ -80,7 +80,7 @@ export class FieldBuilder extends Disposable { private readonly _readonly: Computed; public constructor(public readonly gristDoc: GristDoc, public readonly field: ViewFieldRec, - private _cursor: Cursor) { + private _cursor: Cursor, private _options: { isPreview?: boolean } = {}) { super(); this._docModel = gristDoc.docModel; @@ -89,7 +89,8 @@ export class FieldBuilder extends Disposable { this._readOnlyPureType = ko.pureComputed(() => this.field.column().pureType()); - this._readonly = Computed.create(this, (use) => use(gristDoc.isReadonly) || use(field.disableEditData)); + this._readonly = Computed.create(this, (use) => + use(gristDoc.isReadonly) || use(field.disableEditData) || Boolean(this._options.isPreview)); // Observable with a list of available types. this._availableTypes = Computed.create(this, (use) => { @@ -428,7 +429,7 @@ export class FieldBuilder extends Disposable { this._rowMap.set(row, elem); dom(elem, dom.autoDispose(widgetObs), - kd.cssClass(this.field.formulaCssClass), + this._options.isPreview ? null : kd.cssClass(this.field.formulaCssClass), kd.toggleClass("readonly", toKo(ko, this._readonly)), kd.maybe(isSelected, () => dom('div.selected_cursor', kd.toggleClass('active_cursor', isActive)