Skip to content

Commit

Permalink
(core) Fix setting xaxis when both chart aggregation and split series
Browse files Browse the repository at this point in the history
Summary:
 - Symptoms where that Split Series could end up being turned off for
   no good reason. Also both x axis and split series could be mixed
   up.

 - Problems was caused by call to `setGroupByColumns` which modifies
   the sections viewFields. Diff fixes it by adjustin slightly the
   ordering of function call in `_setXAxis()`.

 - Problem of mixing up x axis and split series was fixed by being
   careful on the order of columns passed to the `setGroupByColumns`
   which then determine the ordering of the view fields.

Test Plan: Includes new test cases

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D3365
  • Loading branch information
cpind committed Apr 13, 2022
1 parent 09da815 commit 25e40bf
Showing 1 changed file with 11 additions and 11 deletions.
22 changes: 11 additions & 11 deletions app/client/components/ChartView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -692,15 +692,6 @@ export class ChartConfig extends GrainJSDisposable {
this._freezeXAxis.set(true);
try {

// if values aggregation is 'on' update the grouped by columns first. This will make sure
// that colId is not missing from the summary table's columns (as could happen if it were a
// non-numeric for instance).
if (this._optionsObj.prop('aggregate')()) {
const splitColId = this._groupDataColId.get();
const cols = splitColId === colId ? [colId] : [colId, splitColId];
await this._setGroupByColumns(cols);
}

// first remove the current field
if (this._xAxisFieldIndex.get() !== -1 && this._xAxisFieldIndex.get() < viewFields.peek().length) {
await this._configFieldsHelper.removeField(viewFields.peek()[this._xAxisFieldIndex.get()]);
Expand All @@ -709,13 +700,22 @@ export class ChartConfig extends GrainJSDisposable {
// if x axis was undefined, set option to false
await setSaveValue(this._optionsObj.prop('isXAxisUndefined'), false);

// if new field was used to group by column series, disable multiseries
// if new field was used to split series, disable multiseries
const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().colId() === colId);
if (fieldIndex === 0 && optionsObj.prop('multiseries').peek()) {
await optionsObj.prop('multiseries').setAndSave(false);
return;
}

// if values aggregation is 'on' update the grouped by columns before findColumn()
// call. This will make sure that colId is not missing from the summary table's columns (as
// could happen if it were a non-numeric for instance).
if (this._optionsObj.prop('aggregate')()) {
const splitColId = this._groupDataColId.get();
const cols = splitColId === colId ? [colId] : [splitColId, colId];
await this._setGroupByColumns(cols);
}

// if the new column for the x axis is already visible, make it the first visible column,
// else add it as the first visible field. The field will be first because it will be
// inserted before current xAxis column (which is already first (or second if we have
Expand Down Expand Up @@ -754,7 +754,7 @@ export class ChartConfig extends GrainJSDisposable {
// non-numeric for instance).
if (this._optionsObj.prop('aggregate')()) {
const xAxisColId = this._xAxis.get();
const cols = xAxisColId === colId ? [colId] : [xAxisColId, colId];
const cols = xAxisColId === colId ? [colId] : [colId, xAxisColId];
await this._setGroupByColumns(cols);
}

Expand Down

0 comments on commit 25e40bf

Please sign in to comment.