From 4fcdd2ba0769223c5177eb64b2241fa632a9ae8b Mon Sep 17 00:00:00 2001 From: Cyprien P Date: Thu, 16 Sep 2021 10:33:56 +0200 Subject: [PATCH] (core) Fix y-axis blinking in chart view configuration Summary: This is a follow up diff for https://phab.getgrist.com/D3021. Y-axis draggable list used to blink when user changed either one of the x axis or groupdata column. This was due to the fact that all of theses axis are stored into the same array and changing one of them changes the whole array even though items relative to the y-axis actually were not changing. This diff addresses this issue by 1) being carefull at not updating the array of items when the changes do not impact y axis. And 2) by adding a freeze observable allowing to freeze the draggable list of y-axis while actions are being treated on the server. Test Plan: Catching such bug is hard, and given that it's only look and fill, maybe not worth the time and effort. Tested manually though. Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3023 --- app/client/components/ChartView.ts | 77 ++++++++++++++++------------ app/client/ui/VisibleFieldsConfig.ts | 15 +++++- 2 files changed, 58 insertions(+), 34 deletions(-) diff --git a/app/client/components/ChartView.ts b/app/client/components/ChartView.ts index f3cc4e55..9046232a 100644 --- a/app/client/components/ChartView.ts +++ b/app/client/components/ChartView.ts @@ -330,6 +330,8 @@ export class ChartConfig extends GrainJSDisposable { // observable is part of a hack to fix this issue. private _freezeXAxis = Observable.create(this, false); + private _freezeYAxis = Observable.create(this, false); + // The column is of the x-axis. private _xAxis: Computed = Computed.create( this, this._xAxisFieldIndex, this._freezeXAxis, (use, i, freeze) => { @@ -381,21 +383,6 @@ export class ChartConfig extends GrainJSDisposable { if (this._section.parentKey() !== 'chart') { return null; } - // The y-axis are all visible fields that comes after the x-axis and maybe the group data - // column. Hence the draggable list of y-axis needs to skip either one or two visible fields. - const skipFirst = Computed.create(this, fromKo(this._optionsObj.prop('multiseries')), (_use, multiseries) => ( - multiseries ? 2 : 1 - )); - - // The draggable list of y-axis - const fieldsDraggable = this._configFieldsHelper.buildVisibleFieldsConfigHelper({ - itemCreateFunc: (field) => this._buildField(field), - draggableOptions: { - removeButton: false, - drag_indicator: cssDragger, - }, skipFirst - }); - return [ cssRow( select(fromKoSave(this._section.chartTypeDef), [ @@ -460,7 +447,7 @@ export class ChartConfig extends GrainJSDisposable { ), cssLabel('SERIES'), - fieldsDraggable, + this._buildYAxis(), cssRow( cssAddYAxis( cssAddIcon('Plus'), 'Add Series', @@ -480,25 +467,30 @@ export class ChartConfig extends GrainJSDisposable { const viewFields = this._section.viewFields.peek(); await this._gristDoc.docData.bundleActions('selected new x-axis', async () => { - // first remove the current field - if (this._xAxisFieldIndex.get() < viewFields.peek().length) { - await this._configFieldsHelper.removeField(viewFields.peek()[this._xAxisFieldIndex.get()]); - } + this._freezeYAxis.set(true); + try { + // first remove the current field + if (this._xAxisFieldIndex.get() < viewFields.peek().length) { + await this._configFieldsHelper.removeField(viewFields.peek()[this._xAxisFieldIndex.get()]); + } - // if new field was used to group by column series, disable multiseries - const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().getRowId() === colId); - if (fieldIndex === 0 && optionsObj.prop('multiseries').peek()) { - await optionsObj.prop('multiseries').setAndSave(false); - return; - } + // if new field was used to group by column series, disable multiseries + const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().getRowId() === colId); + if (fieldIndex === 0 && optionsObj.prop('multiseries').peek()) { + await optionsObj.prop('multiseries').setAndSave(false); + return; + } - // if new field is already visible, moves the fields to the first place else add the field to the first - // place - const xAxisField = viewFields.peek()[this._xAxisFieldIndex.get()]; - if (fieldIndex > -1) { - await this._configFieldsHelper.changeFieldPosition(viewFields.peek()[fieldIndex], xAxisField); - } else { - await this._configFieldsHelper.addField(col, xAxisField); + // if new field is already visible, moves the fields to the first place else add the field to the first + // place + const xAxisField = viewFields.peek()[this._xAxisFieldIndex.get()]; + if (fieldIndex > -1) { + await this._configFieldsHelper.changeFieldPosition(viewFields.peek()[fieldIndex], xAxisField); + } else { + await this._configFieldsHelper.addField(col, xAxisField); + } + } finally { + this._freezeYAxis.set(false); } }); } @@ -508,6 +500,7 @@ export class ChartConfig extends GrainJSDisposable { await this._gristDoc.docData.bundleActions('selected new x-axis', async () => { this._freezeXAxis.set(true); + this._freezeYAxis.set(true); try { // if grouping was already set, first remove the current field if (this._groupDataColId.get() > -1) { @@ -530,6 +523,7 @@ export class ChartConfig extends GrainJSDisposable { await this._optionsObj.prop('multiseries').setAndSave(colId > -1); } finally { this._freezeXAxis.set(false); + this._freezeYAxis.set(false); } }); } @@ -545,6 +539,23 @@ export class ChartConfig extends GrainJSDisposable { testId('y-axis'), ); } + + private _buildYAxis() { + + // The y-axis are all visible fields that comes after the x-axis and maybe the group data + // column. Hence the draggable list of y-axis needs to skip either one or two visible fields. + const skipFirst = Computed.create(this, fromKo(this._optionsObj.prop('multiseries')), (_use, multiseries) => ( + multiseries ? 2 : 1 + )); + + return this._configFieldsHelper.buildVisibleFieldsConfigHelper({ + itemCreateFunc: (field) => this._buildField(field), + draggableOptions: { + removeButton: false, + drag_indicator: cssDragger, + }, skipFirst, freeze: this._freezeYAxis + }); + } } function cssCheckboxRow(label: string, value: KoSaveableObservable, ...args: DomElementArg[]) { diff --git a/app/client/ui/VisibleFieldsConfig.ts b/app/client/ui/VisibleFieldsConfig.ts index 07c6f428..621dc35a 100644 --- a/app/client/ui/VisibleFieldsConfig.ts +++ b/app/client/ui/VisibleFieldsConfig.ts @@ -13,6 +13,7 @@ import { icon } from "app/client/ui2018/icons"; import * as gutil from 'app/common/gutil'; import { Computed, Disposable, dom, IDomArgs, makeTestId, Observable, styled, subscribe } from "grainjs"; import difference = require("lodash/difference"); +import isEqual = require("lodash/isEqual"); const testId = makeTestId('test-vfc-'); @@ -27,6 +28,11 @@ interface DraggableFieldsOption { // the group-by-column in the chart type widget. skipFirst?: Observable; + // Allows to prevent updates of the list. This option is to be used when skipFirst option is used + // and it is useful to prevent the list to update during changes that only affect the skipped + // fields. + freeze?: Observable; + // the itemCreateFunc callback passed to kf.draggableList for the visible fields. itemCreateFunc(field: IField): Element|undefined; } @@ -93,14 +99,21 @@ export class VisibleFieldsConfig extends Disposable { let fields = this._section.viewFields.peek(); if (options.skipFirst) { + const freeze = options.freeze; const allFields = this._section.viewFields.peek(); const newArray = new KoArray(); function update() { - newArray.assign(allFields.peek().filter((_v, i) => i + 1 > options.skipFirst!.get())); + if (freeze && freeze.get()) { return; } + const newValues = allFields.peek().filter((_v, i) => i + 1 > options.skipFirst!.get()); + if (isEqual(newArray.all(), newValues)) { return; } + newArray.assign(newValues); } update(); this.autoDispose(allFields.subscribe(update)); this.autoDispose(subscribe(options.skipFirst, update)); + if (options.freeze) { + this.autoDispose(subscribe(options.freeze, update)); + } fields = newArray; }