From 5c67e12aa5b4f92afc6f81cf8b94a0ce45c0a193 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Thu, 10 Nov 2022 12:59:24 -0500 Subject: [PATCH] (core) When a checkbox is clicked on a new record, set default values determined by linking Summary: Fixes a bug (reported in https://community.getgrist.com/t/bug-toggle-column-in-linking-widget-not-triggering-default-value/1657) Test Plan: Added a test case that fails without this fix. Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3699 --- app/client/components/Cursor.ts | 4 +- app/client/models/DataRowModel.ts | 2 +- app/client/widgets/CheckBox.js | 38 ------------- app/client/widgets/Switch.js | 37 ------------ app/client/widgets/Toggle.ts | 66 ++++++++++++++++++++++ app/client/widgets/UserTypeImpl.ts | 7 +-- test/nbrowser/MultiColumn.ts | 23 +++----- test/nbrowser/ToggleColumns.ts | 90 ++++++++++++++++++++++++++++++ test/nbrowser/gristUtils.ts | 16 ++++++ 9 files changed, 185 insertions(+), 98 deletions(-) delete mode 100644 app/client/widgets/CheckBox.js delete mode 100644 app/client/widgets/Switch.js create mode 100644 app/client/widgets/Toggle.ts create mode 100644 test/nbrowser/ToggleColumns.ts diff --git a/app/client/components/Cursor.ts b/app/client/components/Cursor.ts index a5017209..63203254 100644 --- a/app/client/components/Cursor.ts +++ b/app/client/components/Cursor.ts @@ -54,9 +54,9 @@ export class Cursor extends Disposable { // Command to be manually triggered on cell selection. Moves the cursor to the selected cell. // This is overridden by the formula editor to insert "$col" variables when clicking cells. - setCursor(this: Cursor, rowModel: BaseRowModel, colModel: BaseRowModel) { + setCursor(this: Cursor, rowModel: BaseRowModel, fieldModel: BaseRowModel) { this.rowIndex(rowModel ? rowModel._index() : 0); - this.fieldIndex(colModel ? colModel._index()! : 0); + this.fieldIndex(fieldModel ? fieldModel._index()! : 0); }, }; diff --git a/app/client/models/DataRowModel.ts b/app/client/models/DataRowModel.ts index eab7571b..169415c4 100644 --- a/app/client/models/DataRowModel.ts +++ b/app/client/models/DataRowModel.ts @@ -21,7 +21,7 @@ export class DataRowModel extends BaseRowModel { public _validationFailures: ko.PureComputed>>; public _isAddRow: ko.Observable; - private _isRealChange: ko.Observable; + public _isRealChange: ko.Observable; public constructor(dataTableModel: DataTableModel, colNames: string[]) { super(dataTableModel, colNames); diff --git a/app/client/widgets/CheckBox.js b/app/client/widgets/CheckBox.js deleted file mode 100644 index 1386f989..00000000 --- a/app/client/widgets/CheckBox.js +++ /dev/null @@ -1,38 +0,0 @@ -var dom = require('../lib/dom'); -var dispose = require('../lib/dispose'); -var _ = require('underscore'); -var kd = require('../lib/koDom'); -var AbstractWidget = require('./AbstractWidget'); - -/** - * CheckBox - A bi-state CheckBox widget - */ -function CheckBox(field) { - AbstractWidget.call(this, field, {defaultTextColor: '#606060'}); -} -dispose.makeDisposable(CheckBox); -_.extend(CheckBox.prototype, AbstractWidget.prototype); - -CheckBox.prototype.buildConfigDom = function() { - return null; -}; - -CheckBox.prototype.buildDom = function(row) { - var value = row[this.field.colId()]; - return dom('div.field_clip', - dom('div.widget_checkbox', - dom.on('click', () => { - if (!this.field.column().isRealFormula()) { - value.setAndSave(!value.peek()); - } - }), - dom('div.widget_checkmark', - kd.show(value), - dom('div.checkmark_kick'), - dom('div.checkmark_stem') - ) - ) - ); -}; - -module.exports = CheckBox; diff --git a/app/client/widgets/Switch.js b/app/client/widgets/Switch.js deleted file mode 100644 index 79fa8260..00000000 --- a/app/client/widgets/Switch.js +++ /dev/null @@ -1,37 +0,0 @@ -var dom = require('../lib/dom'); -var dispose = require('../lib/dispose'); -var _ = require('underscore'); -var kd = require('../lib/koDom'); -var AbstractWidget = require('./AbstractWidget'); - -/** - * Switch - A bi-state Switch widget - */ -function Switch(field) { - AbstractWidget.call(this, field, {defaultTextColor: '#2CB0AF'}); -} -dispose.makeDisposable(Switch); -_.extend(Switch.prototype, AbstractWidget.prototype); - -Switch.prototype.buildConfigDom = function() { - return null; -}; - -Switch.prototype.buildDom = function(row) { - var value = row[this.field.colId()]; - return dom('div.field_clip', - dom('div.widget_switch', - kd.toggleClass('switch_on', value), - kd.toggleClass('switch_transition', row._isRealChange), - dom('div.switch_slider'), - dom('div.switch_circle'), - dom.on('click', () => { - if (!this.field.column().isRealFormula()) { - value.setAndSave(!value.peek()); - } - }) - ) - ); -}; - -module.exports = Switch; diff --git a/app/client/widgets/Toggle.ts b/app/client/widgets/Toggle.ts new file mode 100644 index 00000000..e4e3c0d7 --- /dev/null +++ b/app/client/widgets/Toggle.ts @@ -0,0 +1,66 @@ +import * as commands from 'app/client/components/commands'; +import { DataRowModel } from 'app/client/models/DataRowModel'; +import { ViewFieldRec } from 'app/client/models/entities/ViewFieldRec'; +import { KoSaveableObservable } from 'app/client/models/modelUtil'; +import { NewAbstractWidget, Options } from 'app/client/widgets/NewAbstractWidget'; +import { dom } from 'grainjs'; + +/** + * ToggleBase - The base class for toggle widgets, such as a checkbox or a switch. + */ +abstract class ToggleBase extends NewAbstractWidget { + protected _addClickEventHandler(row: DataRowModel) { + return dom.on('click', (event) => { + if (event.shiftKey) { + // Shift-click is for selection, don't also toggle the checkbox during it. + return; + } + if (!this.field.column().isRealFormula()) { + // Move the cursor here, and pretend that spacebar was pressed. This triggers an editing + // flow which is handled by CheckBoxEditor.skipEditor(). This way the edit applies to + // editRow, which handles setting default values based on widget linking. + commands.allCommands.setCursor.run(row, this.field); + commands.allCommands.input.run(' '); + } + }); + } +} + +export class ToggleCheckBox extends ToggleBase { + constructor(field: ViewFieldRec, _options: Options = {}) { + super(field, {defaultTextColor: '#606060'}); + } + + public buildDom(row: DataRowModel) { + const value = row.cells[this.field.colId.peek()] as KoSaveableObservable; + return dom('div.field_clip', + dom('div.widget_checkbox', + dom('div.widget_checkmark', + dom.show(value), + dom('div.checkmark_kick'), + dom('div.checkmark_stem') + ), + this._addClickEventHandler(row), + ) + ); + } +} + +export class ToggleSwitch extends ToggleBase { + constructor(field: ViewFieldRec, _options: Options = {}) { + super(field, {defaultTextColor: '#2CB0AF'}); + } + + public buildDom(row: DataRowModel) { + const value = row.cells[this.field.colId.peek()] as KoSaveableObservable; + return dom('div.field_clip', + dom('div.widget_switch', + dom.cls('switch_on', value), + dom.cls('switch_transition', row._isRealChange), + dom('div.switch_slider'), + dom('div.switch_circle'), + this._addClickEventHandler(row), + ) + ); + } +} diff --git a/app/client/widgets/UserTypeImpl.ts b/app/client/widgets/UserTypeImpl.ts index 23607c29..55b9498d 100644 --- a/app/client/widgets/UserTypeImpl.ts +++ b/app/client/widgets/UserTypeImpl.ts @@ -1,6 +1,5 @@ import {AttachmentsEditor} from 'app/client/widgets/AttachmentsEditor'; import {AttachmentsWidget} from 'app/client/widgets/AttachmentsWidget'; -import CheckBox from 'app/client/widgets/CheckBox'; import CheckBoxEditor from 'app/client/widgets/CheckBoxEditor'; import ChoiceEditor from 'app/client/widgets/ChoiceEditor'; import {ChoiceListCell} from 'app/client/widgets/ChoiceListCell'; @@ -22,7 +21,7 @@ import {ReferenceEditor} from 'app/client/widgets/ReferenceEditor'; import {ReferenceList} from 'app/client/widgets/ReferenceList'; import {ReferenceListEditor} from 'app/client/widgets/ReferenceListEditor'; import {Spinner} from 'app/client/widgets/Spinner'; -import Switch from 'app/client/widgets/Switch'; +import {ToggleCheckBox, ToggleSwitch} from 'app/client/widgets/Toggle'; import {getWidgetConfiguration} from 'app/client/widgets/UserType'; import {GristType} from 'app/plugin/GristData'; @@ -36,10 +35,10 @@ export const nameToWidget = { 'HyperLinkTextBox': HyperLinkTextBox, 'HyperLinkEditor': HyperLinkEditor, 'Spinner': Spinner, - 'CheckBox': CheckBox, + 'CheckBox': ToggleCheckBox, 'CheckBoxEditor': CheckBoxEditor, 'Reference': Reference, - 'Switch': Switch, + 'Switch': ToggleSwitch, 'ReferenceEditor': ReferenceEditor, 'ReferenceList': ReferenceList, 'ReferenceListEditor': ReferenceListEditor, diff --git a/test/nbrowser/MultiColumn.ts b/test/nbrowser/MultiColumn.ts index a1815ed0..022fe6da 100644 --- a/test/nbrowser/MultiColumn.ts +++ b/test/nbrowser/MultiColumn.ts @@ -686,20 +686,20 @@ describe('MultiColumn', function() { assert.isFalse(await widgetTypeDisabled()); // Test for mixed format. await selectColumns('Left'); - await widgetType('TextBox'); + await gu.setFieldWidgetType('TextBox'); await selectColumns('Right'); - await widgetType('CheckBox'); + await gu.setFieldWidgetType('CheckBox'); await selectColumns('Left', 'Right'); - assert.equal(await widgetType(), 'Mixed format'); + assert.equal(await gu.getFieldWidgetType(), 'Mixed format'); // Test that both change when format is changed. for (const mode of ['TextBox', 'CheckBox', 'Switch']) { - await widgetType(mode); + await gu.setFieldWidgetType(mode); await selectColumns('Left'); - assert.equal(await widgetType(), mode); + assert.equal(await gu.getFieldWidgetType(), mode); await selectColumns('Right'); - assert.equal(await widgetType(), mode); + assert.equal(await gu.getFieldWidgetType(), mode); await selectColumns('Left', 'Right'); - assert.equal(await widgetType(), mode); + assert.equal(await gu.getFieldWidgetType(), mode); } } else { await selectColumns('Left', 'Right'); @@ -1127,15 +1127,6 @@ async function alignment(value?: 'left' | 'right' | 'center') { } -async function widgetType(type?: string) { - if (!type) { - return await driver.find(".test-fbuilder-widget-select").getText(); - } - await driver.find(".test-fbuilder-widget-select").click(); - await driver.findContent('.test-select-menu li', gu.exactMatch(type)).click(); - await gu.waitForServer(); -} - async function dateFormatDisabled() { const format = await driver.find('[data-test-id=Widget_dateFormat]'); return await format.matches(".disabled"); diff --git a/test/nbrowser/ToggleColumns.ts b/test/nbrowser/ToggleColumns.ts new file mode 100644 index 00000000..56edad89 --- /dev/null +++ b/test/nbrowser/ToggleColumns.ts @@ -0,0 +1,90 @@ +import {assert, driver} from 'mocha-webdriver'; +import * as gu from 'test/nbrowser/gristUtils'; +import {Session} from 'test/nbrowser/gristUtils'; +import {setupTestSuite} from 'test/nbrowser/testUtils'; + +describe('ToggleColumns', function() { + this.timeout(20000); + + const cleanup = setupTestSuite({team: true}); + + let session: Session; + let docId: string; + + before(async function() { + session = await gu.session().teamSite.login(); + docId = await session.tempNewDoc(cleanup, 'ToggleColumns', {load: false}); + + // Set up a table Src, and table Items which links to Src and has a boolean column. + const api = session.createHomeApi(); + await api.applyUserActions(docId, [ + ['AddTable', 'Src', [{id: 'A', type: 'Text'}]], + ['BulkAddRecord', 'Src', [null, null, null], {A: ['a', 'b', 'c']}], + ['AddTable', 'Items', [ + {id: 'A', type: 'Ref:Src'}, + {id: 'Chk', type: 'Bool'}, + // An extra text column reflects the boolean for simpler checking of the values. + {id: 'Chk2', isFormula: true, formula: '$Chk'}, + ]], + ['BulkAddRecord', 'Items', [null, null, null], {A: [1, 1, 3]}], + ]); + + await session.loadDoc(`/doc/${docId}`); + + // Set up a page with linked widgets. + await gu.addNewPage('Table', 'Src'); + await gu.addNewSection('Table', 'Items', {selectBy: /Src/i}); + }); + + it('should fill in values determined by linking when checkbox is clicked', async function() { + // Test the behavior with a checkbox. + await verifyToggleBehavior(); + }); + + it('should fill in values determined by linking when switch widget is clicked', async function() { + // Now switch the widget to the "Switch" widget, and test again. + await gu.toggleSidePanel('right', 'open'); + await driver.find('.test-right-tab-field').click(); + await gu.setFieldWidgetType("Switch"); + await verifyToggleBehavior(); + }); + + async function verifyToggleBehavior() { + // Selecting a cell in Src should show only linked values in Items. + await gu.getCell({section: 'Src', col: 'A', rowNum: 1}).click(); + assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Chk2'], rowNums: [1, 2, 3]}), [ + 'Src[1]', 'false', + 'Src[1]', 'false', + '', '', + ]); + // Click on the cell in the "Add Row" of Items. Because the checkbox is centered in the cell, + // the click should toggle it. + await gu.getCell({section: 'Items', col: 'Chk', rowNum: 3}).click(); + await gu.waitForServer(); + + // Check that there is a new row, properly linked. + assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Chk2'], rowNums: [1, 2, 3, 4]}), [ + 'Src[1]', 'false', + 'Src[1]', 'false', + 'Src[1]', 'true', + '', '', + ]); + + // Try another row of table Src. It should have its own Items (initially none). + await gu.getCell({section: 'Src', col: 'A', rowNum: 2}).click(); + assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Chk2'], rowNums: [1]}), + ['', '']); + + // Click checkbox in "Add Row" of Items again. + await gu.getCell({section: 'Items', col: 'Chk', rowNum: 1}).click(); + await gu.waitForServer(); + + // Check that we see the new row, with the value determined by linking (column 'A') set correctly. + assert.deepEqual(await gu.getVisibleGridCells({section: 'Items', cols: ['A', 'Chk2'], rowNums: [1, 2]}), [ + 'Src[2]', 'true', + '', '', + ]); + + await gu.undo(2); + } +}); diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index d6f7963a..d010defa 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -1537,6 +1537,22 @@ export async function getType() { return await driver.find('.test-fbuilder-type-select').getText(); } +/** + * Get the field's widget type (e.g. "CheckBox" for a Toggle column) in the creator panel. + */ +export async function getFieldWidgetType(): Promise { + return await driver.find(".test-fbuilder-widget-select").getText(); +} + +/** + * Set the field's widget type (e.g. "CheckBox" for a Toggle column) in the creator panel. + */ +export async function setFieldWidgetType(type: string) { + await driver.find(".test-fbuilder-widget-select").click(); + await driver.findContent('.test-select-menu li', exactMatch(type)).click(); + await waitForServer(); +} + export async function applyTypeTransform() { await driver.findContent('.type_transform_prompt button', /Apply/).click(); }