From d2077bc486a638481caf9967ffbb30c21ca97b15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Tue, 18 Jan 2022 12:48:57 +0100 Subject: [PATCH] (core) Improving experience when editing group-by column. Summary: Improving experience when editing group-by column: - Disable column rename - Allow changing most widget properties: - Color/Background - Number format - Date/DateTime format (but not the timezone) - All toggle options (for toggle column) - Remove Edit button on Choice Edit - Changing the underlying column should reset all those options back to the original column. Test Plan: nbrowser Reviewers: alexmojaki Reviewed By: alexmojaki Subscribers: alexmojaki Differential Revision: https://phab.getgrist.com/D3216 --- app/client/components/GridView.js | 7 +- app/client/lib/ACSelect.ts | 5 ++ app/client/ui/FieldConfig.ts | 7 +- app/client/widgets/ChoiceListEntry.ts | 21 ++++-- app/client/widgets/ChoiceTextBox.ts | 5 +- app/client/widgets/DateTimeTextBox.js | 5 +- app/client/widgets/TZAutocomplete.ts | 5 +- sandbox/grist/test_summary.py | 94 +++++++++++++++++++++++++++ sandbox/grist/useractions.py | 28 +++++++- test/nbrowser/gristUtils.ts | 34 +++++++++- 10 files changed, 196 insertions(+), 15 deletions(-) diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index be72065a..9eed2cd1 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -918,7 +918,12 @@ GridView.prototype.buildDom = function() { kd.style('borderLeftWidth', v.borderWidthPx), kd.foreach(v.viewFields(), field => { var isEditingLabel = ko.pureComputed({ - read: () => this.gristDoc.isReadonlyKo() || self.isPreview ? false : editIndex() === field._index(), + read: () => { + const goodIndex = () => editIndex() === field._index(); + const isReadonly = () => this.gristDoc.isReadonlyKo() || self.isPreview; + const isSummary = () => Boolean(field.column().disableEditData()); + return goodIndex() && !isReadonly() && !isSummary(); + }, write: val => editIndex(val ? field._index() : -1) }).extend({ rateLimit: 0 }); let filterTriggerCtl; diff --git a/app/client/lib/ACSelect.ts b/app/client/lib/ACSelect.ts index a65d5a87..354f6b61 100644 --- a/app/client/lib/ACSelect.ts +++ b/app/client/lib/ACSelect.ts @@ -18,6 +18,7 @@ export interface ACSelectItem extends ACItem { export function buildACSelect( owner: IDisposableOwner, options: { + disabled?: Observable, acIndex: ACIndex, valueObs: Observable, save: (value: string, item: ACSelectItem|undefined) => Promise|void @@ -55,6 +56,9 @@ export function buildACSelect( const onMouseDown = (ev: MouseEvent) => { ev.preventDefault(); // Don't let it affect focus, since we focus/blur manually. + if (options.disabled?.get()) { + return; + } if (!isOpen()) { textInput.focus(); } openOrCommit(); }; @@ -73,6 +77,7 @@ export function buildACSelect( dom.prop('value', valueObs), dom.on('focus', (ev, elem) => elem.select()), dom.on('blur', commitOrRevert), + dom.prop("disabled", (use) => options.disabled ? use(options.disabled) : false), dom.onKeyDown({ Escape: revert, Enter: openOrCommit, diff --git a/app/client/ui/FieldConfig.ts b/app/client/ui/FieldConfig.ts index 2a86a9e6..a6a37fdf 100644 --- a/app/client/ui/FieldConfig.ts +++ b/app/client/ui/FieldConfig.ts @@ -58,7 +58,8 @@ export function buildNameConfig(owner: MultiHolder, origColumn: ColumnRec, curso cssColTieConnectors(), cssToggleButton(icon('FieldReference'), cssToggleButton.cls('-selected', (use) => !use(untieColId)), - dom.on('click', () => untieColId.saveOnly(!untieColId.peek())), + dom.on('click', () => !origColumn.disableModify.peek() && untieColId.saveOnly(!untieColId.peek())), + cssToggleButton.cls("-disabled", origColumn.disableModify), testId('field-derive-id') ), ) @@ -391,6 +392,10 @@ const cssToggleButton = styled(cssIconButton, ` &-selected:hover { --icon-color: ${colors.darkGrey}; } + &-disabled, &-disabled:hover { + --icon-color: ${colors.light}; + background-color: var(--grist-color-medium-grey-opaque); + } `); const cssColLabelBlock = styled('div', ` diff --git a/app/client/widgets/ChoiceListEntry.ts b/app/client/widgets/ChoiceListEntry.ts index 1ec74195..fcfbac8d 100644 --- a/app/client/widgets/ChoiceListEntry.ts +++ b/app/client/widgets/ChoiceListEntry.ts @@ -91,7 +91,8 @@ export class ChoiceListEntry extends Disposable { constructor( private _values: Observable, private _choiceOptionsByName: Observable, - private _onSave: (values: string[], choiceOptions: ChoiceOptionsByName, renames: Record) => void + private _onSave: (values: string[], choiceOptions: ChoiceOptionsByName, renames: Record) => void, + private _disabled: Observable ) { super(); @@ -177,12 +178,15 @@ export class ChoiceListEntry extends Disposable { ) ), dom.on('click', () => this._startEditing()), + cssListBoxInactive.cls("-disabled", this._disabled), testId('choice-list-entry') ), - cssButtonRow( - primaryButton('Edit', - dom.on('click', () => this._startEditing()), - testId('choice-list-entry-edit') + dom.maybe(use => !use(this._disabled), () => + cssButtonRow( + primaryButton('Edit', + dom.on('click', () => this._startEditing()), + testId('choice-list-entry-edit') + ) ) ) ); @@ -191,7 +195,9 @@ export class ChoiceListEntry extends Disposable { } private _startEditing(): void { - this._isEditing.set(true); + if (!this._disabled.get()) { + this._isEditing.set(true); + } } private _save(): void { @@ -369,6 +375,9 @@ const cssListBoxInactive = styled(cssListBox, ` &:hover { border: 1px solid ${colors.hover}; } + &-disabled { + cursor: default; + } `); const cssListRow = styled('div', ` diff --git a/app/client/widgets/ChoiceTextBox.ts b/app/client/widgets/ChoiceTextBox.ts index dbc9763e..cae32ee8 100644 --- a/app/client/widgets/ChoiceTextBox.ts +++ b/app/client/widgets/ChoiceTextBox.ts @@ -5,7 +5,7 @@ import {KoSaveableObservable} from 'app/client/models/modelUtil'; import {cssLabel, cssRow} from 'app/client/ui/RightPanel'; import {testId} from 'app/client/ui2018/cssVars'; import {NTextBox} from 'app/client/widgets/NTextBox'; -import {Computed, dom, styled} from 'grainjs'; +import {Computed, dom, fromKo, styled} from 'grainjs'; import {choiceToken, DEFAULT_FILL_COLOR, DEFAULT_TEXT_COLOR} from 'app/client/widgets/ChoiceToken'; export interface IChoiceOptions { @@ -73,7 +73,8 @@ export class ChoiceTextBox extends NTextBox { ChoiceListEntry, this._choiceValues, this._choiceOptionsByName, - this.save.bind(this) + this.save.bind(this), + fromKo(this.field.column().disableEditData) ) ) ]; diff --git a/app/client/widgets/DateTimeTextBox.js b/app/client/widgets/DateTimeTextBox.js index 4adaa965..5ab8ab03 100644 --- a/app/client/widgets/DateTimeTextBox.js +++ b/app/client/widgets/DateTimeTextBox.js @@ -66,7 +66,10 @@ DateTimeTextBox.prototype.buildConfigDom = function(isTransformConfig) { var self = this; return dom('div', cssLabel("Timezone"), - cssRow(gdom.create(buildTZAutocomplete, moment, fromKo(this._timezone), this._setTimezone)), + cssRow( + gdom.create(buildTZAutocomplete, moment, fromKo(this._timezone), this._setTimezone, + { disabled : fromKo(this.field.column().disableEditData)}), + ), self.buildDateConfigDom(), cssLabel("Time Format"), cssRow(dom(select(fromKo(self.standardTimeFormat), self.timeFormatOptions), dom.testId("Widget_timeFormat"))), diff --git a/app/client/widgets/TZAutocomplete.ts b/app/client/widgets/TZAutocomplete.ts index 01950d8d..8dc108f9 100644 --- a/app/client/widgets/TZAutocomplete.ts +++ b/app/client/widgets/TZAutocomplete.ts @@ -47,7 +47,8 @@ export function buildTZAutocomplete( owner: IDisposableOwner, moment: MomentTimezone, valueObs: Observable, - save: (value: string) => Promise|void + save: (value: string) => Promise|void, + options?: { disabled?: Observable } ) { // Set a large maxResults, since it's sometimes nice to see all supported timezones (there are // fewer than 1000 in practice). @@ -69,7 +70,7 @@ export function buildTZAutocomplete( } }; return buildACSelect(owner, - {acIndex, valueObs, save: saveTZ}, + {...options, acIndex, valueObs, save: saveTZ}, testId("tz-autocomplete") ); } diff --git a/sandbox/grist/test_summary.py b/sandbox/grist/test_summary.py index 53b948cf..b98fd6d1 100644 --- a/sandbox/grist/test_summary.py +++ b/sandbox/grist/test_summary.py @@ -8,6 +8,7 @@ import logger import summary import testutil import test_engine +from useractions import allowed_summary_change from test_engine import Table, Column, View, Section, Field @@ -839,3 +840,96 @@ class Address: [ 1, 1.0, [1,2], 2, 9 ], [ 2, 2.0, [3], 1, 6 ], ]) + + #---------------------------------------------------------------------- + # pylint: disable=R0915 + def test_allow_select_by_change(self): + def widgetOptions(n, o): + return allowed_summary_change('widgetOptions', n, o) + + # Can make no update on widgetOptions. + new = None + old = None + self.assertTrue(widgetOptions(new, old)) + + new = '' + old = None + self.assertTrue(widgetOptions(new, old)) + + new = '' + old = '' + self.assertTrue(widgetOptions(new, old)) + + new = None + old = '' + self.assertTrue(widgetOptions(new, old)) + + # Can update when key was not present + new = '{"widget":"TextBox","alignment":"center"}' + old = '' + self.assertTrue(widgetOptions(new, old)) + + new = '' + old = '{"widget":"TextBox","alignment":"center"}' + self.assertTrue(widgetOptions(new, old)) + + # Can update when key was present. + new = '{"widget":"TextBox","alignment":"center"}' + old = '{"widget":"Spinner","alignment":"center"}' + self.assertTrue(widgetOptions(new, old)) + + # Can update but must leave other options. + new = '{"widget":"TextBox","cant":"center"}' + old = '{"widget":"Spinner","cant":"center"}' + self.assertTrue(widgetOptions(new, old)) + + # Can't add protected property when old was empty. + new = '{"widget":"TextBox","cant":"new"}' + old = None + self.assertFalse(widgetOptions(new, old)) + + # Can't remove when there was a protected property. + new = None + old = '{"widget":"TextBox","cant":"old"}' + self.assertFalse(widgetOptions(new, old)) + + # Can't update by omitting. + new = '{"widget":"TextBox"}' + old = '{"widget":"TextBox","cant":"old"}' + self.assertFalse(widgetOptions(new, old)) + + # Can't update by changing. + new = '{"widget":"TextBox","cant":"new"}' + old = '{"widget":"TextBox","cant":"old"}' + self.assertFalse(widgetOptions(new, old)) + + # Can't update by adding. + new = '{"widget":"TextBox","cant":"new"}' + old = '{"widget":"TextBox"}' + self.assertFalse(widgetOptions(new, old)) + + # Can update objects + new = '{"widget":"TextBox","alignment":{"prop":1},"cant":{"prop":1}}' + old = '{"widget":"TextBox","alignment":{"prop":2},"cant":{"prop":1}}' + self.assertTrue(widgetOptions(new, old)) + + # Can't update objects + new = '{"widget":"TextBox","cant":{"prop":1}}' + old = '{"widget":"TextBox","cant":{"prop":2}}' + self.assertFalse(widgetOptions(new, old)) + + # Can't update lists + new = '{"widget":"TextBox","cant":[1, 2]}' + old = '{"widget":"TextBox","cant":[2, 1]}' + self.assertFalse(widgetOptions(new, old)) + + # Can update lists + new = '{"widget":"TextBox","alignment":[1, 2]}' + old = '{"widget":"TextBox","alignment":[3, 2]}' + self.assertTrue(widgetOptions(new, old)) + + # Can update without changing list. + new = '{"widget":"TextBox","dontChange":[1, 2]}' + old = '{"widget":"Spinner","dontChange":[1, 2]}' + self.assertTrue(widgetOptions(new, old)) + # pylint: enable=R0915 diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 95bd308e..49db3cae 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -146,6 +146,32 @@ def guess_type(values, convert=False): return "Numeric" if total and counter[True] >= total * 0.9 else "Text" +def allowed_summary_change(key, updated, original): + """ + Checks if summary group by column can be modified. + """ + if updated == original: + return True + elif key == 'widgetOptions': + try: + updated_options = json.loads(updated or '{}') + original_options = json.loads(original or '{}') + except ValueError: + return False + # Unfortunately all widgetOptions are allowed to change, except choice items. But it is + # better to list those that can be changed. + # TODO: move choice items to separate column + allowed_to_change = {'widget', 'dateFormat', 'timeFormat', 'isCustomDateFormat', 'alignment', + 'fillColor', 'textColor', 'isCustomTimeFormat', 'isCustomDateFormat', + 'numMode', 'numSign', 'decimals', 'maxDecimals', 'currency'} + # Helper function to remove protected keys from dictionary. + def trim(options): + return {k: v for k, v in options.items() if k not in allowed_to_change} + return trim(updated_options) == trim(original_options) + else: + return False + + class UserActions(object): def __init__(self, eng): self._engine = eng @@ -574,7 +600,7 @@ class UserActions(object): # Type sometimes must differ (e.g. ChoiceList -> Choice). expected = summary.summary_groupby_col_type(expected) - if value != expected: + if not allowed_summary_change(key, value, expected): raise ValueError("Cannot modify summary group-by column '%s'" % col.colId) make_acl_updates = acl.prepare_acl_col_renames(self._docmodel, self, renames) diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 8ddbfec7..29cbabbc 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -890,6 +890,38 @@ export async function undo(optCount: number = 1, optTimeout?: number) { } +/** + * Returns a function to undo all user actions from a particular point in time. + */ +export async function begin() { + const undoStackPointer = () => driver.executeScript(` + return window.gristDocPageModel.gristDoc.get()._undoStack._pointer; + `); + const start = await undoStackPointer(); + return async () => undo(await undoStackPointer() - start); +} + +/** + * Simulates a transaction on the GristDoc. Use with cautions, as there is no guarantee it will undo correctly + * in a case of failure. + * + * Example: + * + * it('should ...', revertChanges(async function() { + * ... + * })); + */ +export function revertChanges(test: () => Promise) { + return async function() { + const revert = await begin(); + try { + await test(); + } finally { + await revert(); + } + }; +} + /** * Click the Redo button and wait for server. If optCount is given, click Redo that many times. */ @@ -1749,7 +1781,7 @@ export async function getDateFormat(): Promise { /** * Changes date format for date and datetime editor */ -export async function setDateFormat(format: string) { +export async function setDateFormat(format: string|RegExp) { await driver.find('[data-test-id=Widget_dateFormat]').click(); await driver.findContentWait('.test-select-menu .test-select-row', format, 200).click(); await waitForServer();