From 6460c22a89646d81a58e4c72ef440565ee6a4287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Fri, 21 Oct 2022 12:55:01 +0200 Subject: [PATCH] (core) Changing shortcuts for adding and removing rows Summary: New shortcuts for removing and adding rows. For adding a row we now have Mod+(Shift)+Enter For removing rows we now have Mod+Delete/Mod+Backspace Before removing rows, the user is prompted to confirm, this prompt can be dismissed and this setting can be remembered. User needs to confirm only when using shortcut. Old shortcuts are still active and shows information about this change. This information is shown only once, after this shortcuts have default behavior (zooming). New users don't see this explanation. Test Plan: Updated Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3655 --- app/client/components/BaseView.js | 77 ++++++++--- app/client/components/DeprecatedCommands.ts | 118 ++++++++++++++++ app/client/components/DetailView.js | 49 +++---- app/client/components/GridView.js | 61 +++++---- app/client/components/GristDoc.ts | 13 +- app/client/components/commandList.js | 26 +++- app/client/components/commands.js | 6 +- app/client/components/modals.ts | 141 ++++++++++++++++++++ app/client/declarations.d.ts | 1 + app/client/models/AppModel.ts | 23 +++- app/client/models/NotifyModel.ts | 16 ++- app/client/models/errors.ts | 12 +- app/client/ui/App.ts | 2 +- app/client/ui/NotifyUI.ts | 4 + app/client/ui2018/modals.ts | 33 +++++ app/common/Prefs.ts | 26 ++++ app/server/lib/FlexServer.ts | 3 + test/nbrowser/ReferenceColumns.ts | 6 +- test/nbrowser/ReferenceList.ts | 8 +- test/nbrowser/gristUtils.ts | 49 +++++++ test/nbrowser/testUtils.ts | 13 +- 21 files changed, 582 insertions(+), 105 deletions(-) create mode 100644 app/client/components/DeprecatedCommands.ts create mode 100644 app/client/components/modals.ts diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 0c35ac0d..0099c68e 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -1,21 +1,23 @@ -var _ = require('underscore'); -var ko = require('knockout'); -var moment = require('moment-timezone'); -var {getSelectionDesc} = require('app/common/DocActions'); -var {nativeCompare, roundDownToMultiple, waitObs} = require('app/common/gutil'); -var gutil = require('app/common/gutil'); +/* globals KeyboardEvent */ + +const _ = require('underscore'); +const ko = require('knockout'); +const moment = require('moment-timezone'); +const {getSelectionDesc} = require('app/common/DocActions'); +const {nativeCompare, roundDownToMultiple, waitObs} = require('app/common/gutil'); +const gutil = require('app/common/gutil'); const MANUALSORT = require('app/common/gristTypes').MANUALSORT; -var gristTypes = require('app/common/gristTypes'); -var tableUtil = require('../lib/tableUtil'); -var {DataRowModel} = require('../models/DataRowModel'); -var {DynamicQuerySet} = require('../models/QuerySet'); -var {SortFunc} = require('app/common/SortFunc'); -var rowset = require('../models/rowset'); -var Base = require('./Base'); -var {Cursor} = require('./Cursor'); -var FieldBuilder = require('../widgets/FieldBuilder'); -var commands = require('./commands'); -var BackboneEvents = require('backbone').Events; +const gristTypes = require('app/common/gristTypes'); +const tableUtil = require('../lib/tableUtil'); +const {DataRowModel} = require('../models/DataRowModel'); +const {DynamicQuerySet} = require('../models/QuerySet'); +const {SortFunc} = require('app/common/SortFunc'); +const rowset = require('../models/rowset'); +const Base = require('./Base'); +const {Cursor} = require('./Cursor'); +const FieldBuilder = require('../widgets/FieldBuilder'); +const commands = require('./commands'); +const BackboneEvents = require('backbone').Events; const {ClientColumnGetters} = require('app/client/models/ClientColumnGetters'); const {reportError, reportSuccess} = require('app/client/models/errors'); const {urlState} = require('app/client/models/gristUrlState'); @@ -26,7 +28,9 @@ const {ExtraRows} = require('app/client/models/DataTableModelWithDiff'); const {createFilterMenu} = require('app/client/ui/ColumnFilterMenu'); const {closeRegisteredMenu} = require('app/client/ui2018/menus'); const {COMMENTS} = require('app/client/models/features'); - +const {DismissedPopup} = require('app/common/Prefs'); +const {markAsSeen} = require('app/client/models/UserPrefs'); +const {buildConfirmDelete, reportUndo} = require('app/client/components/modals'); /** * BaseView forms the basis for ViewSection classes. @@ -241,11 +245,48 @@ BaseView.commonCommands = { showRawData: function() { this.showRawData().catch(reportError); }, + deleteRecords: function(source) { this.deleteRecords(source); }, + filterByThisCellValue: function() { this.filterByThisCellValue(); }, duplicateRows: function() { this._duplicateRows().catch(reportError); }, openDiscussion: function() { this.openDiscussionAtCursor(); }, }; +BaseView.prototype.selectedRows = function() { + return []; +}; + +BaseView.prototype.deleteRows = function(rowIds) { + return this.tableModel.sendTableAction(['BulkRemoveRecord', rowIds]); +}; + +BaseView.prototype.deleteRecords = function(source) { + const rowIds = this.selectedRows(); + if (this.viewSection.disableAddRemoveRows() || rowIds.length === 0){ + return; + } + const isKeyboard = source instanceof KeyboardEvent; + const popups = this.gristDoc.docPageModel.appModel.dismissedPopups; + const popupName = DismissedPopup.check('deleteRecords'); + const onSave = async (remember) => { + if (remember) { + markAsSeen(popups, popupName); + } + return this.deleteRows(rowIds); + }; + if (isKeyboard && !popups.get().includes(popupName)) { + // If we can't find it, use viewPane itself + this.scrollToCursor(); + const selectedCell = this.viewPane.querySelector(".selected_cursor") || this.viewPane; + buildConfirmDelete(selectedCell, onSave, rowIds.length <= 1); + } else { + onSave().then(() => { + reportUndo(this.gristDoc, `You deleted ${rowIds.length} row${rowIds.length > 1 ? 's' : ''}.`); + return true; + }); + } +}; + /** * Sets the cursor to the given position, deferring if necessary until the current query finishes * loading. diff --git a/app/client/components/DeprecatedCommands.ts b/app/client/components/DeprecatedCommands.ts new file mode 100644 index 00000000..a66baa55 --- /dev/null +++ b/app/client/components/DeprecatedCommands.ts @@ -0,0 +1,118 @@ +import * as commands from 'app/client/components/commands'; +import {Command} from 'app/client/components/commands'; +import {markAsSeen} from 'app/client/models/UserPrefs'; +import {get as getBrowserGlobals} from 'app/client/lib/browserGlobals'; +import {reportMessage} from 'app/client/models/errors'; +import {DeprecationWarning} from 'app/common/Prefs'; +import {GristDoc} from 'app/client/components/GristDoc'; +import {showDeprecatedWarning} from 'app/client/components/modals'; +import {Disposable, dom, Holder, styled} from 'grainjs'; +import intersection from "lodash/intersection"; + +const G = getBrowserGlobals('document', 'window'); + +/** + * Manages deprecated commands and keyboard shortcuts. It subscribes itself to all commands and + * keyboard shortcuts, and shows a warning when a deprecated command is used. + */ +export class DeprecatedCommands extends Disposable { + // Holds the commands created by this class, so they can be disposed, + // when this class is disposed or reattached. + private _holder = Holder.create(this); + + constructor(private _gristDoc: GristDoc) { + super(); + G.window.resetSeenWarnings = () => {}; + } + + public attach() { + // We can be attached multiple times, so first clear previous commands. + this._holder.clear(); + + // Get all the warnings from the app model and expose reset function (used in tests only). + // When we reset the warnings, we also need to reattach ourselves. + const seenWarnings = this._gristDoc.docPageModel.appModel.deprecatedWarnings; + G.window.resetSeenWarnings = () => { + if (!this._gristDoc.isDisposed()) { + seenWarnings.set([]); + this.attach(); + } + }; + + // We wan't do anything for anonymous users. + if (!this._gristDoc.docPageModel.appModel.currentValidUser) { + return; + } + + // If user has seen all keyboard warnings, don't need to do anything. + const commandList = Object.values(commands.allCommands as Record); + const deprecatedCommands = commandList.filter((command) => command.deprecated); + const deprecatedNames = deprecatedCommands.map((command) => command.name); + if (intersection(seenWarnings.get(), deprecatedNames).length === deprecatedNames.length) { + return; + } + + // Now subscribe to all the commands and handle them. + const group: any = {}; + for (const c of deprecatedCommands) { + group[c.name] = this._handleCommand.bind(this, c); + } + if (Object.keys(group).length) { + this._holder.autoDispose(commands.createGroup(group, this, true)); + } + } + + private _handleCommand(c: Command) { + const seenWarnings = this._gristDoc.docPageModel.appModel.deprecatedWarnings; + if (!this._hasSeenWarning(c.name)) { + markAsSeen(seenWarnings, c.name); + this._showWarning(c.desc); + return false; // Stop processing. + } else { + return true; // Continue processing. + } + } + + private _showWarning(desc: string) { + // Try to figure out where to show the message. If we have active view, we can try + // to find the selected cell and show the message there. Otherwise, we show it in the + // bottom right corner as a warning. + const selectedCell = this._gristDoc.currentView.get()?.viewPane.querySelector(".selected_cursor"); + if (!selectedCell) { + reportMessage(() => dom('div', this._createMessage(desc)), { + level: 'info', + key: 'deprecated-command', + }); + } else { + showDeprecatedWarning(selectedCell, this._createMessage(desc)); + } + } + + private _hasSeenWarning(name: string) { + const seenWarnings = this._gristDoc.docPageModel.appModel.deprecatedWarnings; + const preference = seenWarnings.get() ?? []; + return preference.includes(DeprecationWarning.check(name)); + } + + private _createMessage(description: string) { + const elements: Node[] = []; + // Description can have embedded commands in the form of {commandName}. + // To construct message we need to replace all {name} to key strokes dom. + for (const part of description.split(/({\w+})/g)) { + // If it starts with { + if (part[0] === '{') { + const otherCommand = commands.allCommands[part.slice(1, -1)]; + if (otherCommand) { + elements.push(otherCommand.getKeysDom()); + } + } else { + elements.push(cssTallerText(part)); + } + } + return elements; + } +} + +const cssTallerText = styled('span', ` + line-height: 24px; +`); diff --git a/app/client/components/DetailView.js b/app/client/components/DetailView.js index 1a3db0c2..6054a53d 100644 --- a/app/client/components/DetailView.js +++ b/app/client/components/DetailView.js @@ -1,18 +1,18 @@ -var _ = require('underscore'); -var ko = require('knockout'); +const _ = require('underscore'); +const ko = require('knockout'); -var dom = require('app/client/lib/dom'); -var kd = require('app/client/lib/koDom'); -var koDomScrolly = require('app/client/lib/koDomScrolly'); +const dom = require('app/client/lib/dom'); +const kd = require('app/client/lib/koDom'); +const koDomScrolly = require('app/client/lib/koDomScrolly'); const {renderAllRows} = require('app/client/components/Printing'); require('app/client/lib/koUtil'); // Needed for subscribeInit. -var Base = require('./Base'); -var BaseView = require('./BaseView'); -var {CopySelection} = require('./CopySelection'); -var RecordLayout = require('./RecordLayout'); -var commands = require('./commands'); +const Base = require('./Base'); +const BaseView = require('./BaseView'); +const {CopySelection} = require('./CopySelection'); +const RecordLayout = require('./RecordLayout'); +const commands = require('./commands'); const {RowContextMenu} = require('../ui/RowContextMenu'); const {parsePasteForView} = require("./BaseView2"); @@ -122,14 +122,6 @@ DetailView.generalCommands = { cursorDown: function() { this.cursor.fieldIndex(this.cursor.fieldIndex() + 1); }, pageUp: function() { this.cursor.rowIndex(this.cursor.rowIndex() - 1); }, pageDown: function() { this.cursor.rowIndex(this.cursor.rowIndex() + 1); }, - - deleteRecords: function() { - // Do not allow deleting the add record row. - if (!this._isAddRow()) { - this.deleteRow(this.cursor.rowIndex()); - } - }, - copy: function() { return this.copy(this.getSelection()); }, cut: function() { return this.cut(this.getSelection()); }, paste: function(pasteObj, cutCallback) { @@ -146,18 +138,21 @@ DetailView.generalCommands = { //---------------------------------------------------------------------- -// TODO: Factor code duplicated with GridView for deleteRow, deleteColumn, -// insertDetailField out of the view modules -DetailView.prototype.deleteRow = function(index) { - if (this.viewSection.disableAddRemoveRows()) { - return; +DetailView.prototype.selectedRows = function() { + if (!this._isAddRow()) { + return [this.viewData.getRowId(this.cursor.rowIndex())]; } - var action = ['RemoveRecord', this.viewData.getRowId(index)]; - return this.tableModel.sendTableAction(action) - .bind(this).then(function() { + return []; +}; + +DetailView.prototype.deleteRows = async function(rowIds) { + const index = this.cursor.rowIndex(); + try { + await BaseView.prototype.deleteRows.call(this, rowIds); + } finally { this.cursor.rowIndex(index); - }); + } }; /** diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index d09bdccf..2f1e745d 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -1,4 +1,4 @@ -/* globals alert, $ */ +/* globals $ */ const _ = require('underscore'); const ko = require('knockout'); @@ -26,7 +26,9 @@ const koUtil = require('app/client/lib/koUtil'); const convert = require('color-convert'); const {renderAllRows} = require('app/client/components/Printing'); -const {reportError} = require('app/client/models/AppModel'); +const {reportWarning} = require('app/client/models/errors'); +const {reportUndo} = require('app/client/components/modals'); + const {onDblClickMatchElem} = require('app/client/lib/dblclick'); // Grist UI Components @@ -46,7 +48,6 @@ const {showTooltip} = require('app/client/ui/tooltips'); const {parsePasteForView} = require("./BaseView2"); const {CombinedStyle} = require("app/client/models/Styles"); - // A threshold for interpreting a motionless click as a click rather than a drag. // Anything longer than this time (in milliseconds) should be interpreted as a drag // even if there is no movement. @@ -299,25 +300,19 @@ GridView.gridCommands = { // Re-define editField after fieldEditSave to make it take precedence for the Enter key. editField: function() { closeRegisteredMenu(); this.scrollToCursor(true); this.activateEditorAtCursor(); }, - deleteRecords: function() { - const saved = this.cursor.getCursorPos(); - this.cursor.setLive(false); - - // Don't return a promise. Nothing will use it, and the Command implementation will not - // prevent default browser behavior if we return a truthy value. - this.deleteRows(this.getSelection()) - .finally(() => { - this.cursor.setCursorPos(saved); - this.cursor.setLive(true); - this.clearSelection(); - }) - .catch(reportError); - }, insertFieldBefore: function() { this.insertColumn(this.cursor.fieldIndex()); }, insertFieldAfter: function() { this.insertColumn(this.cursor.fieldIndex() + 1); }, renameField: function() { this.currentEditingColumnIndex(this.cursor.fieldIndex()); }, hideFields: function() { this.hideFields(this.getSelection()); }, - deleteFields: function() { this.deleteColumns(this.getSelection()); }, + deleteFields: function() { + const selection = this.getSelection(); + const count = selection.colIds.length; + this.deleteColumns(selection).then((result) => { + if (result !== false) { + reportUndo(this.gristDoc, `You deleted ${count} column${count > 1 ? 's' : ''}.`); + } + }); + }, clearValues: function() { this.clearValues(this.getSelection()); }, clearColumns: function() { this._clearColumns(this.getSelection()); }, convertFormulasToData: function() { this._convertFormulasToData(this.getSelection()); }, @@ -670,14 +665,21 @@ GridView.prototype.preventAssignCursor = function() { this._assignCursorTimeoutId = null; } -GridView.prototype.deleteRows = function(selection) { - if (!this.viewSection.disableAddRemoveRows()) { - var rowIds = _.without(selection.rowIds, 'new'); - if (rowIds.length > 0) { - return this.tableModel.sendTableAction(['BulkRemoveRecord', rowIds]); - } +GridView.prototype.selectedRows = function() { + const selection = this.getSelection(); + return _.without(selection.rowIds, 'new'); +}; + +GridView.prototype.deleteRows = async function(rowIds) { + const saved = this.cursor.getCursorPos(); + this.cursor.setLive(false); + try { + await BaseView.prototype.deleteRows.call(this, rowIds); + } finally { + this.cursor.setCursorPos(saved); + this.cursor.setLive(true); + this.clearSelection(); } - return Promise.resolve(); }; GridView.prototype.addNewColumn = function() { @@ -728,14 +730,17 @@ GridView.prototype.showColumn = function(colId, index) { GridView.prototype.deleteColumns = function(selection) { var fields = selection.fields; if (fields.length === this.viewSection.viewFields().peekLength) { - alert("You can't delete all the columns on the grid."); - return; + reportWarning("You can't delete all the columns on the grid.", { + key: 'delete-all-columns', + }); + return Promise.resolve(false); } let actions = fields.filter(col => !col.disableModify()).map(col => ['RemoveColumn', col.colId()]); if (actions.length > 0) { - this.tableModel.sendTableActions(actions, `Removed columns ${actions.map(a => a[1]).join(', ')} ` + + return this.tableModel.sendTableActions(actions, `Removed columns ${actions.map(a => a[1]).join(', ')} ` + `from ${this.tableModel.tableData.tableId}.`).then(() => this.clearSelection()); } + return Promise.resolve(false); }; GridView.prototype.hideFields = function(selection) { diff --git a/app/client/components/GristDoc.ts b/app/client/components/GristDoc.ts index 5ded57a8..37bb0081 100644 --- a/app/client/components/GristDoc.ts +++ b/app/client/components/GristDoc.ts @@ -11,6 +11,7 @@ import {CodeEditorPanel} from 'app/client/components/CodeEditorPanel'; import * as commands from 'app/client/components/commands'; import {CursorPos} from 'app/client/components/Cursor'; import {CursorMonitor, ViewCursorPos} from "app/client/components/CursorMonitor"; +import {DeprecatedCommands} from 'app/client/components/DeprecatedCommands'; import {DocComm} from 'app/client/components/DocComm'; import * as DocConfigTab from 'app/client/components/DocConfigTab'; import {Drafts} from "app/client/components/Drafts"; @@ -60,6 +61,7 @@ import {isList, isListType, isRefListType, RecalcWhen} from 'app/common/gristTyp import {HashLink, IDocPage, isViewDocPage, SpecialDocPage, ViewDocPage} from 'app/common/gristUrls'; import {undef, waitObs} from 'app/common/gutil'; import {LocalPlugin} from "app/common/plugin"; +import {DismissedPopup} from 'app/common/Prefs'; import {StringUnion} from 'app/common/StringUnion'; import {TableData} from 'app/common/TableData'; import {DocStateComparison} from 'app/common/UserAPI'; @@ -329,8 +331,8 @@ export class GristDoc extends DisposableWithEvents { /* Command binding */ this.autoDispose(commands.createGroup({ - undo() { this._undoStack.sendUndoAction().catch(reportError); }, - redo() { this._undoStack.sendRedoAction().catch(reportError); }, + undo(this: GristDoc) { this._undoStack.sendUndoAction().catch(reportError); }, + redo(this: GristDoc) { this._undoStack.sendRedoAction().catch(reportError); }, reloadPlugins() { this.docComm.reloadPlugins().then(() => G.window.location.reload(false)); }, }, this, true)); @@ -399,6 +401,12 @@ export class GristDoc extends DisposableWithEvents { this.draftMonitor = Drafts.create(this, this); this.cursorMonitor = CursorMonitor.create(this, this); this.editorMonitor = EditorMonitor.create(this, this); + + DeprecatedCommands.create(this, this).attach(); + + G.window.resetSeenPopups = (seen = false) => { + this.docPageModel.appModel.dismissedPopups.set(seen ? DismissedPopup.values : []); + }; } /** @@ -530,6 +538,7 @@ export class GristDoc extends DisposableWithEvents { this.trigger('schemaUpdateAction', docActions); } this.docPageModel.updateCurrentDocUsage(message.data.docUsage); + this.trigger('onDocUserAction', docActions); } } diff --git a/app/client/components/commandList.js b/app/client/components/commandList.js index 867d565d..3d07f1e5 100644 --- a/app/client/components/commandList.js +++ b/app/client/components/commandList.js @@ -1,3 +1,6 @@ +const { DeprecationWarning } = require('app/common/Prefs'); + + // The top-level groups, and the ordering within them are for user-facing documentation. exports.groups = [{ group: 'General', @@ -329,15 +332,15 @@ exports.groups = [{ commands: [ { name: 'insertRecordBefore', - keys: ['Mod+Shift+='], + keys: ['Mod+Shift+Enter'], desc: 'Insert a new record, before the currently selected one in an unsorted table' }, { name: 'insertRecordAfter', - keys: ['Mod+='], + keys: ['Mod+Enter'], desc: 'Insert a new record, after the currently selected one in an unsorted table', }, { name: 'deleteRecords', - keys: ['Mod+-'], + keys: ['Mod+Del', 'Mod+Backspace'], desc: 'Delete the currently selected record' }, { name: 'insertFieldBefore', @@ -383,7 +386,22 @@ exports.groups = [{ name: 'duplicateRows', keys: ['Mod+Shift+d'], desc: 'Duplicate selected rows' - } + }, { + name: DeprecationWarning.parse('deprecatedInsertRowBefore'), + keys: ['Mod+Shift+='], + desc: 'Shortcuts to remove or insert a record have changed, to avoid interfering with page zoom. In the future, to delete a record use {deleteRecords}, and to insert a record use {insertRecordAfter}.', + deprecated: true, + }, { + name: DeprecationWarning.parse('deprecatedInsertRecordAfter'), + keys: ['Mod+='], + desc: 'Shortcuts to remove or insert a record have changed, to avoid interfering with page zoom. In the future, to delete a record use {deleteRecords}, and to insert a record use {insertRecordAfter}.', + deprecated: true, + }, { + name: DeprecationWarning.parse('deprecatedDeleteRecords'), + keys: ['Mod+-'], + desc: 'Shortcuts to remove or insert a record have changed, to avoid interfering with page zoom. In the future, to delete a record use {deleteRecords}, and to insert a record use {insertRecordAfter}.', + deprecated: true, + }, ], }, { group: 'Sorting', diff --git a/app/client/components/commands.js b/app/client/components/commands.js index e846894b..38a91ec2 100644 --- a/app/client/components/commands.js +++ b/app/client/components/commands.js @@ -63,7 +63,7 @@ function init(optCommandGroups) { if (allCommands[c.name]) { console.error("Ignoring duplicate command %s in commandList", c.name); } else { - allCommands[c.name] = new Command(c.name, c.desc, c.keys); + allCommands[c.name] = new Command(c.name, c.desc, c.keys, c.deprecated); } }); }); @@ -116,7 +116,7 @@ function getHumanKey(key, isMac) { * @property {Function} run: A bound function that will run the currently active implementation. * @property {Observable} isActive: Knockout observable for whether this command is active. */ -function Command(name, desc, keys) { +function Command(name, desc, keys, deprecated) { this.name = name; this.desc = desc; this.humanKeys = keys.map(key => getHumanKey(key, isMac)); @@ -124,7 +124,7 @@ function Command(name, desc, keys) { this.isActive = ko.observable(false); this._implGroupStack = []; this._activeFunc = _.noop; // The function to run when this command is invoked. - + this.deprecated = deprecated || false; // Let .run bind the Command object, so that it can be used as a stand-alone callback. this.run = this._run.bind(this); } diff --git a/app/client/components/modals.ts b/app/client/components/modals.ts new file mode 100644 index 00000000..94b683f8 --- /dev/null +++ b/app/client/components/modals.ts @@ -0,0 +1,141 @@ +import * as commands from 'app/client/components/commands'; +import {GristDoc} from 'app/client/components/GristDoc'; +import {FocusLayer} from 'app/client/lib/FocusLayer'; +import {reportSuccess} from 'app/client/models/errors'; +import {basicButton, primaryButton} from 'app/client/ui2018/buttons'; +import {labeledSquareCheckbox} from 'app/client/ui2018/checkbox'; +import {testId, theme} from 'app/client/ui2018/cssVars'; +import {modalTooltip} from 'app/client/ui2018/modals'; +import {dom, DomContents, observable, styled} from 'grainjs'; + +/** + * This is a file for all custom and pre-configured popups, modals, toasts and tooltips, used + * in more then one component. + */ + +/** + * Tooltip or popup to confirm row deletion. + */ +export function buildConfirmDelete( + refElement: Element, + onSave: (remember: boolean) => void, + single = true, +) { + const remember = observable(false); + const tooltip = modalTooltip(refElement, (ctl) => + cssContainer( + dom.autoDispose(remember), + testId('confirm-deleteRows'), + testId('confirm-popup'), + elem => { FocusLayer.create(ctl, {defaultFocusElem: elem, pauseMousetrap: true}); }, + dom.onKeyDown({ + Escape: () => ctl.close(), + Enter: () => { onSave(remember.get()); ctl.close(); }, + }), + dom('div', `Are you sure you want to delete ${single ? 'this' : 'these'} record${single ? '' : 's'}?`, + dom.style('margin-bottom', '10px'), + ), + cssButtons( + dom.style('margin-bottom', '12px'), + primaryButton('Delete', testId('confirm-save'), dom.on('click', () => { + onSave(remember.get()); + ctl.close(); + })), + basicButton('Cancel', testId('confirm-cancel'), dom.on('click', () => ctl.close())) + ), + dom('div', + labeledSquareCheckbox(remember, "Don't ask again.", testId('confirm-remember')), + ) + ), {} + ); + // Attach this tooltip to a cell so that it is automatically closed when the cell is disposed. + // or scrolled out of view (and then disposed). + dom.onDisposeElem(refElement, () => { + if (!tooltip.isDisposed()) { + tooltip.close(); + } + }); + return tooltip; +} + +export function showDeprecatedWarning( + refElement: Element, + content: DomContents +) { + const tooltip = modalTooltip(refElement, (ctl) => + cssWideContainer( + testId('popup-warning-deprecated'), + elem => { FocusLayer.create(ctl, {defaultFocusElem: elem, pauseMousetrap: true}); }, + dom.onKeyDown({ + Escape: () => ctl.close(), + Enter: () => ctl.close(), + }), + content, + cssButtons( + dom.style('margin-top', '12px'), + dom.style('justify-content', 'right'), + basicButton('Close', testId('confirm-cancel'), dom.on('click', () => ctl.close())) + ), + ) + ); + // Attach this warning to a cell so that it is automatically closed when the cell is disposed. + // or scrolled out of view (and then disposed). + dom.onDisposeElem(refElement, () => { + if (!tooltip.isDisposed()) { + tooltip.close(); + } + }); + return tooltip; +} + +/** + * Shows notification with a single button 'Undo' delete. + */ +export function reportUndo( + doc: GristDoc, + messageLabel: string, + buttonLabel = 'Undo to restore' +) { + // First create a notification with a button to undo the delete. + let notification = reportSuccess(messageLabel, { + key: 'undo', + actions: [{ + label: buttonLabel, + action: () => { + // When user clicks on the button, undo the last action. + commands.allCommands.undo.run(); + // And remove this notification. + close(); + }, + }] + }); + + // When we received some actions from the server, cancel this popup, + // as the undo might do something else. + doc.on('onDocUserAction', close); + notification?.onDispose(() => doc.off('onDocUserAction', close)); + + function close() { + if (notification && !notification?.isDisposed()) { + notification.dispose(); + notification = undefined; + } + } +} + +const cssTheme = styled('div', ` + color: ${theme.text}; +`); + +const cssButtons = styled('div', ` + display: flex; + gap: 6px; +`); + +const cssContainer = styled(cssTheme, ` + max-width: 210px; +`); + +const cssWideContainer = styled(cssTheme, ` + max-width: 340px; +`); diff --git a/app/client/declarations.d.ts b/app/client/declarations.d.ts index f9c4aa02..3be3229e 100644 --- a/app/client/declarations.d.ts +++ b/app/client/declarations.d.ts @@ -110,6 +110,7 @@ declare module "app/client/components/ViewConfigTab" { declare module "app/client/components/commands" { export class Command { public name: string; + public deprecated: boolean; public desc: string; public humanKeys: string[]; public keys: string[]; diff --git a/app/client/models/AppModel.ts b/app/client/models/AppModel.ts index 3e668a95..40e9ad2a 100644 --- a/app/client/models/AppModel.ts +++ b/app/client/models/AppModel.ts @@ -11,7 +11,7 @@ import {Features, isLegacyPlan, Product} from 'app/common/Features'; import {GristLoadConfig} from 'app/common/gristUrls'; import {FullUser} from 'app/common/LoginSessionAPI'; import {LocalPlugin} from 'app/common/plugin'; -import {UserPrefs} from 'app/common/Prefs'; +import {DeprecationWarning, DismissedPopup, UserPrefs} from 'app/common/Prefs'; import {isOwner} from 'app/common/roles'; import {getTagManagerScript} from 'app/common/tagManager'; import {getDefaultThemePrefs, Theme, ThemeAppearance, ThemeColors, ThemePrefs, @@ -22,6 +22,7 @@ import {getOrgName, Organization, OrgError, UserAPI, UserAPIImpl} from 'app/comm import {getUserPrefObs, getUserPrefsObs} from 'app/client/models/UserPrefs'; import {bundleChanges, Computed, Disposable, Observable, subscribe} from 'grainjs'; +// Reexported for convenience. export {reportError} from 'app/client/models/errors'; export type PageType = "doc" | "home" | "billing" | "welcome"; @@ -60,8 +61,10 @@ export interface TopAppModel { fetchUsersAndOrgs(): Promise; } -// AppModel is specific to the currently loaded organization and active user. It gets rebuilt when -// we switch the current organization or the current user. +/** + * AppModel is specific to the currently loaded organization and active user. It gets rebuilt when + * we switch the current organization or the current user. + */ export interface AppModel { topAppModel: TopAppModel; api: UserAPI; @@ -83,6 +86,14 @@ export interface AppModel { userPrefsObs: Observable; themePrefs: Observable; currentTheme: Computed; + /** + * Popups that user has seen. + */ + dismissedPopups: Observable; + /** + * Deprecation messages that user has seen. + */ + deprecatedWarnings: Observable; pageType: Observable; @@ -222,6 +233,12 @@ export class AppModelImpl extends Disposable implements AppModel { }) as Observable; public readonly currentTheme = this._getCurrentThemeObs(); + public readonly dismissedPopups = + getUserPrefObs(this.userPrefsObs, 'dismissedPopups', { defaultValue: [] }) as Observable; + public readonly deprecatedWarnings = getUserPrefObs(this.userPrefsObs, 'seenDeprecatedWarnings', + { defaultValue: []}) as Observable; + + // Get the current PageType from the URL. public readonly pageType: Observable = Computed.create(this, urlState().state, (use, state) => (state.doc ? "doc" : (state.billing ? "billing" : (state.welcome ? "welcome" : "home")))); diff --git a/app/client/models/NotifyModel.ts b/app/client/models/NotifyModel.ts index 22eb5dd9..81f8b56b 100644 --- a/app/client/models/NotifyModel.ts +++ b/app/client/models/NotifyModel.ts @@ -26,7 +26,7 @@ interface INotifier { getFullAppErrors(): IAppError[]; } -interface INotification extends Expirable { +export interface INotification extends Expirable { expire(): Promise; } @@ -34,11 +34,19 @@ export interface IProgress extends Expirable { setProgress(percent: number): void; } +/** + * Custom action to be shown as a notification with a handler. + */ +export interface CustomAction { label: string, action: () => void } +/** + * A string, or a function that builds dom. + */ +export type MessageType = string | (() => DomElementArg); // Identifies supported actions. These are implemented in NotifyUI. -export type NotifyAction = 'upgrade' | 'renew' | 'personal' | 'report-problem' | 'ask-for-help'; +export type NotifyAction = 'upgrade' | 'renew' | 'personal' | 'report-problem' | 'ask-for-help' | CustomAction; export interface INotifyOptions { - message: string | (() => DomElementArg); // A string, or a function that builds dom. + message: MessageType; // A string, or a function that builds dom. timestamp?: number; title?: string; canUserClose?: boolean; @@ -224,7 +232,7 @@ export class Notifier extends Disposable implements INotifier { * Additional option level, can be used to style the notification to like a success, warning, * info or error message. */ - public createUserMessage(message: string, options: Partial = {}): INotification { + public createUserMessage(message: MessageType, options: Partial = {}): INotification { const timestamp = Date.now(); if (options.actions && options.actions.includes('ask-for-help')) { // If user should be able to ask for help, add this error to the notifier dropdown too for a diff --git a/app/client/models/errors.ts b/app/client/models/errors.ts index 7def3c36..79ce5ad0 100644 --- a/app/client/models/errors.ts +++ b/app/client/models/errors.ts @@ -1,6 +1,6 @@ import {get as getBrowserGlobals} from 'app/client/lib/browserGlobals'; import * as log from 'app/client/lib/log'; -import {INotifyOptions, Notifier} from 'app/client/models/NotifyModel'; +import {INotification, INotifyOptions, MessageType, Notifier} from 'app/client/models/NotifyModel'; import {ApiErrorDetails} from 'app/common/ApiError'; import {fetchFromHome, pageHasHome} from 'app/common/urlUtils'; import isError = require('lodash/isError'); @@ -44,9 +44,9 @@ export function getAppErrors(): string[] { /** * Shows normal notification without any styling or icon. */ -export function reportMessage(msg: string, options?: Partial) { +export function reportMessage(msg: MessageType, options?: Partial): INotification|undefined { if (_notifier && !_notifier.isDisposed()) { - _notifier.createUserMessage(msg, { + return _notifier.createUserMessage(msg, { ...options }); } @@ -60,14 +60,14 @@ export function reportWarning(msg: string, options?: Partial) { options = {level: 'warning', ...options}; log.warn(`${options.level}: `, msg); _logError(msg); - reportMessage(msg, options); + return reportMessage(msg, options); } /** * Shows success toast notification (with green styling). */ -export function reportSuccess(msg: string, options?: Partial) { - reportMessage(msg, {level: 'success', ...options}); +export function reportSuccess(msg: MessageType, options?: Partial) { + return reportMessage(msg, {level: 'success', ...options}); } /** diff --git a/app/client/ui/App.ts b/app/client/ui/App.ts index 06d9afad..603a4c6f 100644 --- a/app/client/ui/App.ts +++ b/app/client/ui/App.ts @@ -95,7 +95,7 @@ export class App extends DisposableWithEvents { ) ), dom.forEach(commandList.groups, (group: any) => { - const cmds = group.commands.filter((cmd: any) => Boolean(cmd.desc && cmd.keys.length)); + const cmds = group.commands.filter((cmd: any) => Boolean(cmd.desc && cmd.keys.length && !cmd.deprecated)); return cmds.length > 0 ? dom('tbody', dom('tr', diff --git a/app/client/ui/NotifyUI.ts b/app/client/ui/NotifyUI.ts index 8d23dd51..c56a0630 100644 --- a/app/client/ui/NotifyUI.ts +++ b/app/client/ui/NotifyUI.ts @@ -60,6 +60,10 @@ function buildAction(action: NotifyAction, item: Notification, options: IBeaconO return cssToastAction('Ask for help', dom.on('click', () => beaconOpenMessage({...options, includeAppErrors: true, errors}))); } + + default: + return cssToastAction(action.label, testId('toast-custom-action'), + dom.on('click', action.action)); } } diff --git a/app/client/ui2018/modals.ts b/app/client/ui2018/modals.ts index 3b5caf30..b2d8f723 100644 --- a/app/client/ui2018/modals.ts +++ b/app/client/ui2018/modals.ts @@ -6,6 +6,7 @@ import {bigBasicButton, bigPrimaryButton, cssButton} from 'app/client/ui2018/but import {mediaSmall, testId, theme, vars} from 'app/client/ui2018/cssVars'; import {loadingSpinner} from 'app/client/ui2018/loaders'; import {waitGrainObs} from 'app/common/gutil'; +import {IOpenController, IPopupDomCreator, IPopupOptions, PopupControl, popupOpen} from 'popweasel'; import {Computed, Disposable, dom, DomContents, DomElementArg, input, keyframes, MultiHolder, Observable, styled} from 'grainjs'; @@ -468,8 +469,40 @@ export function cssModalWidth(style: ModalWidth) { return cssModalDialog.cls('-' + style); } +/** + * Shows a little modal as a tooltip. + * + * Example: + * dom.on('click', (_, element) => modalTooltip(element, (ctl) => { + * return dom('div', 'Hello world', dom.on('click', () => ctl.close())); + * })) + */ +export function modalTooltip( + reference: Element, + domCreator: IPopupDomCreator, + options: IPopupOptions = {} +): PopupControl { + return popupOpen(reference, (ctl: IOpenController) => { + const element = cssModalTooltip( + domCreator(ctl) + ); + return element; + }, options); +} + /* CSS styled components */ +const cssModalTooltip = styled('div', ` + padding: 16px; + background: ${theme.modalBg}; + border-radius: 3px; + box-shadow: 0 2px 18px 0 ${theme.modalInnerShadow}, 0 0 1px 0 ${theme.modalOuterShadow}; + outline: none; + & > div { + outline: none; + } +`); + // For centering, we use 'margin: auto' on the flex item instead of 'justify-content: center' on // the flex container, to ensure the full item can be scrolled in case of overflow. // See https://stackoverflow.com/a/33455342/328565 diff --git a/app/common/Prefs.ts b/app/common/Prefs.ts index be53e29c..dacf22b2 100644 --- a/app/common/Prefs.ts +++ b/app/common/Prefs.ts @@ -23,6 +23,10 @@ export interface UserPrefs extends Prefs { recordSignUpEvent?: boolean; // Theme-related preferences. theme?: ThemePrefs; + // List of deprecated warnings user have seen. + seenDeprecatedWarnings?: DeprecationWarning[]; + // List of dismissedPopups user have seen. + dismissedPopups?: DismissedPopup[]; } // A collection of preferences related to a combination of user and org. @@ -45,3 +49,25 @@ export interface UserOrgPrefs extends Prefs { } export type OrgPrefs = Prefs; + +/** + * List of all deprecated warnings that user can see and dismiss. + * All of them are marked as seen for new users in FlexServer.ts (welcomeNewUser handler). + * For now we use then to mark which keyboard shortcuts are deprecated, so those keys + * are also used in commandList.js. + */ +export const DeprecationWarning = StringUnion( + 'deprecatedInsertRowBefore', + 'deprecatedInsertRecordAfter', + 'deprecatedDeleteRecords', +); +export type DeprecationWarning = typeof DeprecationWarning.type; + +/** + * List of all popups that user can see and dismiss + */ +export const DismissedPopup = StringUnion( + 'deleteRecords', // confirmation for deleting records keyboard shortcut + 'deleteFields' // confirmation for deleting columns keyboard shortcut +); +export type DismissedPopup = typeof DismissedPopup.type; diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 9da2fb2e..6c3f3be4 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -5,6 +5,7 @@ import {encodeUrl, getSlugIfNeeded, GristLoadConfig, IGristUrlState, isOrgInPath parseSubdomain, sanitizePathTail} from 'app/common/gristUrls'; import {getOrgUrlInfo} from 'app/common/gristUrls'; import {UserProfile} from 'app/common/LoginSessionAPI'; +import {DeprecationWarning} from 'app/common/Prefs'; import {tbind} from 'app/common/tbind'; import * as version from 'app/common/version'; import {ApiServer} from 'app/gen-server/ApiServer'; @@ -812,6 +813,8 @@ export class FlexServer implements GristServer { await this._dbManager.updateOrg(getScope(req), 0, {userPrefs: { showNewUserQuestions: true, recordSignUpEvent: true, + // Mark all deprecated warnings as seen. + seenDeprecatedWarnings: DeprecationWarning.values }}); const domain = mreq.org ?? null; diff --git a/test/nbrowser/ReferenceColumns.ts b/test/nbrowser/ReferenceColumns.ts index b799d193..205d247f 100644 --- a/test/nbrowser/ReferenceColumns.ts +++ b/test/nbrowser/ReferenceColumns.ts @@ -5,7 +5,6 @@ import {server, setupTestSuite} from 'test/nbrowser/testUtils'; describe('ReferenceColumns', function() { this.timeout(20000); - setupTestSuite(); let session: Session; const cleanup = setupTestSuite({team: true}); @@ -543,7 +542,8 @@ describe('ReferenceColumns', function() { // Delete a row. await gu.getCell({section: 'Colors', col: 'Color Name', rowNum: 1}).doClick(); - await driver.find('body').sendKeys(Key.chord(await gu.modKey(), '-')); + await driver.find('body').sendKeys(Key.chord(await gu.modKey(), Key.DELETE)); + await gu.confirm(true, true); await gu.waitForServer(); // See that the value is gone from the autocomplete. @@ -554,7 +554,7 @@ describe('ReferenceColumns', function() { // Add a row. await gu.getCell({section: 'Colors', col: 'Color Name', rowNum: 1}).doClick(); - await driver.find('body').sendKeys(Key.chord(await gu.modKey(), '=')); + await driver.find('body').sendKeys(Key.chord(await gu.modKey(), Key.ENTER)); await gu.waitForServer(); await driver.sendKeys('HELIOTROPE', Key.ENTER); await gu.waitForServer(); diff --git a/test/nbrowser/ReferenceList.ts b/test/nbrowser/ReferenceList.ts index 40c15614..4ab1e1e6 100644 --- a/test/nbrowser/ReferenceList.ts +++ b/test/nbrowser/ReferenceList.ts @@ -154,8 +154,7 @@ describe('ReferenceList', function() { // Now delete the row containing Avatar. await gu.getCell('Title', 4, 'Films record').doClick(); - await gu.sendKeys(Key.chord(await gu.modKey(), '-')); - await gu.waitForServer(); + await gu.removeRow(4); // Check that all references to Avatar are deleted. assert.deepEqual( @@ -840,8 +839,7 @@ describe('ReferenceList', function() { // Delete a row. await gu.getCell({section: 'Colors', col: 'Color Name', rowNum: 1}).doClick(); - await driver.find('body').sendKeys(Key.chord(await gu.modKey(), '-')); - await gu.waitForServer(); + await gu.removeRow(1); // See that the value is gone from the autocomplete. await cell.click(); @@ -851,7 +849,7 @@ describe('ReferenceList', function() { // Add a row. await gu.getCell({section: 'Colors', col: 'Color Name', rowNum: 1}).doClick(); - await driver.find('body').sendKeys(Key.chord(await gu.modKey(), '=')); + await driver.find('body').sendKeys(Key.chord(await gu.modKey(), Key.ENTER)); await gu.waitForServer(); await driver.sendKeys('HELIOTROPE', Key.ENTER); await gu.waitForServer(); diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 530ce88a..4db44f3b 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -411,6 +411,30 @@ export async function getGridRowCount(): Promise { return parseInt(rowNum, 10); } +/** + * Returns the total row count in the card list that is the active section by scrolling to the bottom + * and examining the last row number. The count includes the special "Add Row". + */ +export async function getCardListCount(): Promise { + await sendKeys(Key.chord(await modKey(), Key.DOWN)); + const rowNum = await driver.find('.active.detailview_record_detail .detail_row_num').getText(); + return parseInt(rowNum, 10); +} + +/** + * Returns the total row count in the card widget that is the active section by looking + * at the displayed count in the section header. The count includes the special "Add Row". + */ +export async function getCardCount(): Promise { + const section = await driver.findWait('.active_section', 4000); + const counter = await section.findAll(".grist-single-record__menu__count"); + if (counter.length) { + const cardRow = (await counter[0].getText()).split(' OF ')[1]; + return parseInt(cardRow) + 1; + } + return 1; +} + /** * Return the .column-name element for the specified column, which may be specified by full name * or index, and may include a section (or will use the active section by default). @@ -856,6 +880,22 @@ export async function sendActions(actions: UserAction[]) { await waitForServer(); } +/** + * Confirms dialog for removing rows. In the future, can be used for other dialogs. + */ +export async function confirm(save = true, remember = false) { + if (await driver.find(".test-confirm-save").isPresent()) { + if (remember) { + await driver.find(".test-confirm-remember").click(); + } + if (save) { + await driver.find(".test-confirm-save").click(); + } else { + await driver.find(".test-confirm-cancel").click(); + } + } +} + /** * Returns the left-panel item for the given page, given by a full string name, or a RegExp. * You may simply click it to switch to that page. @@ -2472,6 +2512,15 @@ export async function scrollActiveView(x: number, y: number) { await driver.sleep(10); // wait a bit for the scroll to happen (this is async operation in Grist). } +export async function scrollActiveViewTop() { + await driver.executeScript(function() { + const view = document.querySelector(".active_section .grid_view_data") || + document.querySelector(".active_section .detailview_scroll_pane"); + view!.scrollTop = 0; + }); + await driver.sleep(10); // wait a bit for the scroll to happen (this is async operation in Grist). +} + /** * Filters a column in a Grid using the filter menu. */ diff --git a/test/nbrowser/testUtils.ts b/test/nbrowser/testUtils.ts index 37caa780..1b5a0ff9 100644 --- a/test/nbrowser/testUtils.ts +++ b/test/nbrowser/testUtils.ts @@ -88,7 +88,18 @@ export function setupTestSuite(options?: TestSuiteOptions) { // Also, log out, to avoid logins interacting, unless NO_CLEANUP is requested (useful for // debugging tests). if (!process.env.NO_CLEANUP) { - after(() => server.removeLogin()); + after(async () => { + try { + await server.removeLogin(); + } catch(err) { + // If there are any alerts open, close them as it might be blocking other tests. + if (err.name && err.name === 'UnexpectedAlertOpenError') { + await driver.switchTo().alert().accept(); + assert.fail("Unexpected alert open"); + } + throw err; + } + }); } // If requested, clear user preferences for all test users after this suite.