Skip to content

Commit

Permalink
refactor: Replace usage of Column.index with column name (#1126)
Browse files Browse the repository at this point in the history 
resolves #965

BREAKING CHANGE: Removed index property from dh.types Column type.
IrisGridUtils.dehydrateSort now returns column name instead of index.
TableUtils now expects column name instead of index for functions that
don't have access to a columns array.
  • Loading branch information
@bmingles
bmingles authored Mar 7, 2023
1 parent 6debb74 commit 7448a88
Show file tree
Hide file tree
Showing 10 changed files with 163 additions and 73 deletions.
1 change: 1 addition & 0 deletions .eslintignore
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
__mocks__/dh-core.js
3 changes: 2 additions & 1 deletion packages/dashboard-core-plugins/src/panels/ChartPanel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
DehydratedSort,
DehydratedAdvancedFilter,
DehydratedQuickFilter,
LegacyDehydratedSort,
} from '@deephaven/iris-grid';
import dh, {
FigureDescriptor,
Expand Down Expand Up @@ -107,7 +108,7 @@ export interface ChartPanelTableSettings {
quickFilters?: readonly DehydratedQuickFilter[];
advancedFilters?: readonly DehydratedAdvancedFilter[];
inputFilters?: readonly InputFilter[];
sorts?: readonly DehydratedSort[];
sorts?: readonly (DehydratedSort | LegacyDehydratedSort)[];
partition?: unknown;
partitionColumn?: string;
}
Expand Down
7 changes: 4 additions & 3 deletions packages/iris-grid/src/IrisGrid.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2654,7 +2654,8 @@ export class IrisGrid extends Component<IrisGridProps, IrisGridState> {
const { model } = this.props;
const columnIndex = model.getColumnIndexByName(column.name);
assertNotNull(columnIndex);
const oldSort = TableUtils.getSortForColumn(model.sort, columnIndex);
const columnName = model.columns[columnIndex].name;
const oldSort = TableUtils.getSortForColumn(model.sort, columnName);
let newSort = null;

if (oldSort == null || oldSort.direction !== direction) {
Expand All @@ -2667,7 +2668,7 @@ export class IrisGrid extends Component<IrisGridProps, IrisGridState> {

const sorts = TableUtils.setSortForColumn(
model.sort,
columnIndex,
columnName,
newSort,
addToExisting
);
Expand Down Expand Up @@ -4153,7 +4154,7 @@ export class IrisGrid extends Component<IrisGridProps, IrisGridState> {
const column = model.columns[modelColumn];
const advancedFilter = advancedFilters.get(modelColumn);
const { options: advancedFilterOptions } = advancedFilter || {};
const sort = TableUtils.getSortForColumn(model.sort, modelColumn);
const sort = TableUtils.getSortForColumn(model.sort, column.name);
const sortDirection = sort ? sort.direction : null;
const element = (
<div
Expand Down
8 changes: 7 additions & 1 deletion packages/iris-grid/src/IrisGridRenderer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,13 @@ class IrisGridRenderer extends GridRenderer {
return;
}

const sort = TableUtils.getSortForColumn(model.sort, modelColumn);
const columnName = model.columns[modelColumn]?.name;

if (columnName == null) {
return;
}

const sort = TableUtils.getSortForColumn(model.sort, columnName);

if (!sort) {
return;
Expand Down
95 changes: 60 additions & 35 deletions packages/iris-grid/src/IrisGridUtils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import { DateUtils } from '@deephaven/jsapi-utils';
import type { AdvancedFilter } from './CommonTypes';
import { FilterData } from './IrisGrid';
import IrisGridTestUtils from './IrisGridTestUtils';
import IrisGridUtils from './IrisGridUtils';
import IrisGridUtils, {
DehydratedSort,
LegacyDehydratedSort,
} from './IrisGridUtils';

function makeFilter() {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand All @@ -17,7 +20,7 @@ function makeColumns(count = 30) {

for (let i = 0; i < count; i += 1) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const column = new (dh as any).Column({ index: i, name: `${i}` });
const column = new (dh as any).Column({ index: i, name: `name_${i}` });
columns.push(column);
}

Expand Down Expand Up @@ -151,30 +154,52 @@ describe('sort exporting/importing', () => {
expect(importedSort).toEqual(sort);
});

it('exports/imports sorts', () => {
it('should export (dehydrate) sorts', () => {
const columns = makeColumns();
const sort = [columns[3].sort(), columns[7].sort().abs().desc()];
const table = makeTable({ columns, sort });
const exportedSort = IrisGridUtils.dehydrateSort(sort);
expect(exportedSort).toEqual([
{ column: 3, isAbs: false, direction: 'ASC' },
{ column: 7, isAbs: true, direction: 'DESC' },
]);
const dehydratedSorts = IrisGridUtils.dehydrateSort(sort);

const importedSort = IrisGridUtils.hydrateSort(table.columns, exportedSort);
expect(importedSort).toEqual([
expect.objectContaining({
column: columns[3],
isAbs: false,
direction: 'ASC',
}),
expect.objectContaining({
column: columns[7],
isAbs: true,
direction: 'DESC',
}),
expect(dehydratedSorts).toEqual<DehydratedSort[]>([
{ column: columns[3].name, isAbs: false, direction: 'ASC' },
{ column: columns[7].name, isAbs: true, direction: 'DESC' },
]);
});

describe('should import (hydrate) sorts', () => {
const columns = makeColumns();
const sort = [columns[3].sort(), columns[7].sort().abs().desc()];
const table = makeTable({ columns, sort });

const dehydratedSorts = IrisGridUtils.dehydrateSort(sort);

// Map `column` to a number to represent our LegacyDehydratedSort
const legacyDehydratedSorts: LegacyDehydratedSort[] = dehydratedSorts.map(
({ column, ...rest }) => ({
column: Number(column.split('_')[1]),
...rest,
})
);

it.each([
['current', dehydratedSorts],
['legacy', legacyDehydratedSorts],
])('%s', (_label, sorts) => {
const importedSort = IrisGridUtils.hydrateSort(table.columns, sorts);

expect(importedSort).toEqual([
expect.objectContaining({
column: columns[3],
isAbs: false,
direction: 'ASC',
}),
expect.objectContaining({
column: columns[7],
isAbs: true,
direction: 'DESC',
}),
]);
});
});
});

describe('pendingDataMap hydration/dehydration', () => {
Expand Down Expand Up @@ -222,15 +247,15 @@ describe('pendingDataMap hydration/dehydration', () => {
1,
expect.objectContaining({
data: [
['3', 'Foo'],
['4', 'Bar'],
['name_3', 'Foo'],
['name_4', 'Bar'],
],
}),
],
[
10,
expect.objectContaining({
data: [['7', 'Baz']],
data: [['name_7', 'Baz']],
}),
],
]);
Expand All @@ -256,7 +281,7 @@ describe('remove columns in moved columns', () => {
const newMovedColumns = IrisGridUtils.removeColumnFromMovedColumns(
table.columns,
movedColumns,
['3']
['name_3']
);
expect(newMovedColumns).toEqual([]);
});
Expand All @@ -268,7 +293,7 @@ describe('remove columns in moved columns', () => {
const newMovedColumns = IrisGridUtils.removeColumnFromMovedColumns(
table.columns,
movedColumns,
['3']
['name_3']
);
expect(newMovedColumns).toEqual(GridUtils.moveItem(3, 1, [])); // new move should be {from: 3, to: 1}
});
Expand All @@ -281,7 +306,7 @@ describe('remove columns in moved columns', () => {
const newMovedColumns = IrisGridUtils.removeColumnFromMovedColumns(
table.columns,
movedColumns,
['3']
['name_3']
);
// columns' original state should be [0,1,2,4,5,...] after '3' is removed;
// columns after move should be [4,0,1,2,5,...]; after columns '3' is removed;
Expand All @@ -297,7 +322,7 @@ describe('remove columns in moved columns', () => {
const newMovedColumns = IrisGridUtils.removeColumnFromMovedColumns(
table.columns,
movedColumns,
['4']
['name_4']
);
// column for is removed, the moved column moved back to it's original place, so delete the move
expect(newMovedColumns).toEqual([]);
Expand All @@ -310,7 +335,7 @@ describe('remove columns in moved columns', () => {
const newMovedColumns = IrisGridUtils.removeColumnFromMovedColumns(
table.columns,
movedColumns,
['2', '3']
['name_2', 'name_3']
);
// columns' original state should be [0,1,4,5,...] after '2' & '3' are removed;
// columns after move should be [0,1,4,5,...]; after columns '2' & '3' are removed;
Expand All @@ -325,7 +350,7 @@ describe('remove columns in moved columns', () => {
const newMovedColumns = IrisGridUtils.removeColumnFromMovedColumns(
table.columns,
movedColumns,
['2', '3']
['name_2', 'name_3']
);
// columns' original state should be [0,1,4,5,...] after '2' & '3' are removed;
// columns after move should be [0,4,1,5,...]; after columns '2' & '3' are removed;
Expand All @@ -341,7 +366,7 @@ describe('remove columns in moved columns', () => {
const newMovedColumns = IrisGridUtils.removeColumnFromMovedColumns(
table.columns,
movedColumns,
['2', '3']
['name_2', 'name_3']
);
// columns' original state should be [0,1,4,5,6...] after '2' & '3' are removed;
// columns after moves should be [0,4,1,6,5...]; after columns '2' & '3' are removed;
Expand Down Expand Up @@ -461,9 +486,9 @@ describe('changeFilterColumnNamesToIndexes', () => {
const columns = makeColumns(10);
it('Replaces column names with indexes', () => {
const filters = [
{ name: '1', filter: DEFAULT_FILTER },
{ name: '3', filter: DEFAULT_FILTER },
{ name: '5', filter: DEFAULT_FILTER },
{ name: 'name_1', filter: DEFAULT_FILTER },
{ name: 'name_3', filter: DEFAULT_FILTER },
{ name: 'name_5', filter: DEFAULT_FILTER },
];
expect(
IrisGridUtils.changeFilterColumnNamesToIndexes(columns, filters)
Expand All @@ -477,7 +502,7 @@ describe('changeFilterColumnNamesToIndexes', () => {
it('Omits missing columns', () => {
const filters = [
{ name: 'missing_1', filter: DEFAULT_FILTER },
{ name: '3', filter: DEFAULT_FILTER },
{ name: 'name_3', filter: DEFAULT_FILTER },
{ name: 'missing_2', filter: DEFAULT_FILTER },
];
expect(
Expand Down
26 changes: 19 additions & 7 deletions packages/iris-grid/src/IrisGridUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,18 @@ export type DehydratedUserColumnWidth = [ColumnName, number];

export type DehydratedUserRowHeight = [number, number];

export type DehydratedSort = {
/** @deprecated Use `DehydratedSort` instead */
export interface LegacyDehydratedSort {
column: ModelIndex;
isAbs: boolean;
direction: SortDirection;
};
}

export interface DehydratedSort {
column: ColumnName;
isAbs: boolean;
direction: SortDirection;
}

export interface DehydratedIrisGridState {
advancedFilters: readonly DehydratedAdvancedFilter[];
Expand Down Expand Up @@ -770,7 +777,7 @@ class IrisGridUtils {
return sorts.map(sort => {
const { column, isAbs, direction } = sort;
return {
column: column.index,
column: column.name,
isAbs,
direction,
};
Expand All @@ -785,16 +792,21 @@ class IrisGridUtils {
*/
static hydrateSort(
columns: readonly Column[],
sorts: readonly DehydratedSort[]
sorts: readonly (DehydratedSort | LegacyDehydratedSort)[]
): Sort[] {
return (
sorts
.map(sort => {
const { column: columnIndex, isAbs, direction } = sort;
const { column: columnIndexOrName, isAbs, direction } = sort;
if (direction === TableUtils.sortDirection.reverse) {
return dh.Table.reverse();
}
const column = IrisGridUtils.getColumn(columns, columnIndex);

const column =
typeof columnIndexOrName === 'string'
? IrisGridUtils.getColumnByName(columns, columnIndexOrName)
: IrisGridUtils.getColumn(columns, columnIndexOrName);

if (column != null) {
let columnSort = column.sort();
if (isAbs) {
Expand Down Expand Up @@ -868,7 +880,7 @@ class IrisGridUtils {
quickFilters?: readonly DehydratedQuickFilter[];
advancedFilters?: readonly DehydratedAdvancedFilter[];
inputFilters?: readonly InputFilter[];
sorts?: readonly DehydratedSort[];
sorts?: readonly (DehydratedSort | LegacyDehydratedSort)[];
partition?: unknown;
partitionColumn?: ColumnName;
},
Expand Down
2 changes: 1 addition & 1 deletion packages/iris-grid/src/mousehandlers/IrisGridContextMenuHandler.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ class IrisGridContextMenuHandler extends GridMouseHandler {
} = theme;

const modelSort = model.sort;
const columnSort = TableUtils.getSortForColumn(modelSort, modelIndex);
const columnSort = TableUtils.getSortForColumn(modelSort, column.name);
const hasReverse = reverseType !== TableUtils.REVERSE_TYPE.NONE;
const { userColumnWidths } = metrics;
const isColumnHidden = [...userColumnWidths.values()].some(
Expand Down
5 changes: 0 additions & 5 deletions packages/jsapi-types/src/dh.types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -524,11 +524,6 @@ export interface OneClick {
}

export interface Column {
/**
* @deprecated
*/
readonly index: number;

readonly type: string;
readonly name: string;
readonly description: string;
Expand Down
Loading

0 comments on commit 7448a88

Please sign in to comment.