From e297c8ac2c4a1ffb1aeab0e1bc7a518329f85b9c Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 10:57:07 +0100 Subject: [PATCH 01/29] test: Add a failing test showing the reorder problem As described in #152, the behaviour when reordering over hidden columns is incorrect. This test illustrates the expected behaviour so that we can fix the implementation. --- .../column-reordering/rendering-test.gts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 3a9baef0..9c20c197 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -364,6 +364,28 @@ module('Plugins | columnReordering', function (hooks) { await click('.D.show'); assert.strictEqual(getColumnOrder(), 'B A C D', 'all columns are visible in the correct order'); }); + + test('moving past hidden columns works as expected', async function (assert) { + assert.strictEqual(getColumnOrder(), 'A B C D', 'initially, columns exist as defined'); + + await click('.B.hide') + assert.strictEqual(getColumnOrder(), 'A C D', 'column B is no longer shown, and the order of the remaining columns is retained'); + + await click('th.A .right'); + assert.strictEqual(getColumnOrder(), 'C A D', 'column A was moved to the right'); + + await click('.B.show'); + assert.strictEqual(getColumnOrder(), 'B C A D', 'column B is now shown'); + + await click('.A.hide'); + assert.strictEqual(getColumnOrder(), 'B C D', 'column A is hidden'); + + await click('th.D .left'); + assert.strictEqual(getColumnOrder(), 'B D C', 'column D was moved to the left'); + + await click('.A.show'); + assert.strictEqual(getColumnOrder(), 'B D C A', 'column A has returned, and it is in the right place'); + }); }); module('with a preferences adapter', function (hooks) { From f3e53b341fa0aebfd19fd7b073bb0ec892bcffe1 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 18:18:51 +0100 Subject: [PATCH 02/29] test: Update test because we can and should retain the position of hidden elements --- test-app/tests/plugins/column-reordering/rendering-test.gts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 9c20c197..14299311 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -297,7 +297,7 @@ module('Plugins | columnReordering', function (hooks) { assert.strictEqual(getColumnOrder(), 'A B C D'); }); - test('without setting the order of anything, we cannot retain the order of the columns when they are added or removed', async function (assert) { + test('without setting the order of anything, we retain the order of the columns when they are added or removed', async function (assert) { assert.strictEqual(getColumnOrder(), 'A B C D', 'test scenario is set up'); let columnC = ctx.columns.find(column => column.key === 'C'); @@ -310,7 +310,7 @@ module('Plugins | columnReordering', function (hooks) { ctx.columns = [...ctx.columns, columnC]; await settled(); - assert.strictEqual(getColumnOrder(), 'A B D C', 'column C is restored, but at the end'); + assert.strictEqual(getColumnOrder(), 'A B C D', 'column C is restored in the correct place'); }); test('we can remove and add a column, and a previously set order is retained', async function (assert) { From 6b6d70207e84d209579daf326287afe37ed8ddd0 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 18:19:46 +0100 Subject: [PATCH 03/29] demo: Add an extra column to the demo To make it easier to play with show/hiding then moving columns --- docs/demo/demo-a.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/demo/demo-a.md b/docs/demo/demo-a.md index a7cfff66..4ad80441 100644 --- a/docs/demo/demo-a.md +++ b/docs/demo/demo-a.md @@ -107,6 +107,9 @@ export default class extends Component { { name: 'column C', key: 'C', pluginOptions: [ColumnResizing.forColumn(() => ({ minWidth: 200 }))] }, + { name: 'column D', key: 'D', + pluginOptions: [ColumnResizing.forColumn(() => ({ minWidth: 200 }))] + }, ], data: () => this.data, plugins: [ From 9fc3db3bfb0271d1c8328e4eed2533828207bd53 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 18:31:47 +0100 Subject: [PATCH 04/29] feat: Implement more intuitive column sorting In order to make the previously added test pass we need to give the column reordering plugin access to _all_ columns, not just the visible ones. When moving left or right we need to iteratively swap the position of adjacent columns, continuing until we have swapped with a visible column. There are some failing tests for internals of the component on this commit but they can be fixed up later. I want to see if the new UX is desirable. --- .../src/plugins/column-reordering/plugin.ts | 95 +++++++----- .../column-reordering/ColumnOrder-test.ts | 140 +++++++++++------- .../column-reordering/rendering-test.gts | 8 +- 3 files changed, 153 insertions(+), 90 deletions(-) diff --git a/ember-headless-table/src/plugins/column-reordering/plugin.ts b/ember-headless-table/src/plugins/column-reordering/plugin.ts index f2bea8f3..55e1f9e7 100644 --- a/ember-headless-table/src/plugins/column-reordering/plugin.ts +++ b/ember-headless-table/src/plugins/column-reordering/plugin.ts @@ -61,6 +61,7 @@ export class ColumnMeta { return this.#tableMeta.getPosition(this.column); } + // Swaps this column with the column in the new position set position(value: number) { this.#tableMeta.setPosition(this.column, value); } @@ -109,7 +110,8 @@ export class TableMeta { */ @tracked columnOrder = new ColumnOrder({ - columns: () => this.availableColumns, + allColumns: () => this.allColumns, + availableColumns: () => this.availableColumns, save: this.save, existingOrder: this.read(), }); @@ -145,7 +147,8 @@ export class TableMeta { reset() { preferences.forTable(this.table, ColumnReordering).delete('order'); this.columnOrder = new ColumnOrder({ - columns: () => this.availableColumns, + allColumns: () => this.allColumns, + availableColumns: () => this.availableColumns, save: this.save, }); } @@ -177,15 +180,21 @@ export class TableMeta { } get columns() { - return this.columnOrder.orderedColumns; + return this.columnOrder.orderedColumns.filter((column) => this.availableColumns[column.key]); } - /** - * @private - * This isn't our data to expose, but it is useful to alias - */ private get availableColumns() { - return columns.for(this.table, ColumnReordering); + return columns + .for(this.table, ColumnReordering) + .reduce>((acc, column) => { + acc[column.key] = true; + + return acc; + }, {}); + } + + private get allColumns() { + return this.table.columns.values(); } } @@ -201,13 +210,17 @@ export class ColumnOrder { constructor( private args: { - columns: () => Column[]; + allColumns: () => Column[]; + availableColumns: () => Record; save?: (order: Map) => void; existingOrder?: Map; } ) { if (args.existingOrder) { this.map = new TrackedMap(args.existingOrder); + // TODO: Add anything from `allColumns` that wasn't in `existingOrder` to the end + } else { + this.map = new TrackedMap(args.allColumns().map((column, i) => [column.key, i])); } } @@ -220,16 +233,29 @@ export class ColumnOrder { */ @action moveLeft(key: string) { - let orderedColumns = this.orderedColumns; + if (this.map.get(key) === 0) { + return; + } let found = false; - let nextColumn: { key: string } | undefined; - for (let column of orderedColumns.reverse()) { + for (let column of this.orderedColumns.reverse()) { if (found) { - nextColumn = column; + // Shift moved column left + let currentPosition = this.map.get(key); - break; + assert('current key must exist in map', currentPosition !== undefined); + this.map.set(key, currentPosition - 1); + + // Shift displayed column right + let displayedColumnPosition = this.map.get(column.key); + + assert('displaced key must exist in map', displayedColumnPosition !== undefined); + this.map.set(column.key, displayedColumnPosition + 1); + + if (this.args.availableColumns()[column.key]) { + break; + } } if (column.key === key) { @@ -237,14 +263,11 @@ export class ColumnOrder { } } - if (!nextColumn) return; - - let nextPosition = this.get(nextColumn.key); - - this.swapWith(key, nextPosition); + this.args.save?.(this.map); } setAll = (map: Map) => { + // TODO: Verify that the passed `map` has consectuive values set? this.map.clear(); for (let [key, value] of map.entries()) { @@ -252,6 +275,7 @@ export class ColumnOrder { } this.args.save?.(map); + // TODO: Add anything from `allColumns` that wasn't in the passed `map` to the end }; /** @@ -263,16 +287,25 @@ export class ColumnOrder { */ @action moveRight(key: string) { - let orderedColumns = this.orderedColumns; - let found = false; - let nextColumn: { key: string } | undefined; - for (let column of orderedColumns) { + for (let column of this.orderedColumns) { if (found) { - nextColumn = column; + // Shift moved column right + let currentPosition = this.map.get(key); - break; + assert('current key must exist in map', currentPosition !== undefined); + this.map.set(key, currentPosition + 1); + + // Shift displaced column left + let displayedColumnPosition = this.map.get(column.key); + + assert('displaced key must exist in map', displayedColumnPosition !== undefined); + this.map.set(column.key, displayedColumnPosition - 1); + + if (this.args.availableColumns()[column.key]) { + break; + } } if (column.key === key) { @@ -280,11 +313,7 @@ export class ColumnOrder { } } - if (!nextColumn) return; - - let nextPosition = this.get(nextColumn.key); - - this.swapWith(key, nextPosition); + this.args.save?.(this.map); } /** @@ -312,7 +341,7 @@ export class ColumnOrder { `The current positions are: ` + [...this.orderedMap.entries()].map((entry) => entry.join(' => ')).join(', ') + ` and the availableColumns are: ` + - this.args.columns().map((column) => column.key) + + Object.keys(this.args.availableColumns()) + ` and current "map" (${this.map.size}) is: ` + [...this.map.entries()].map((entry) => entry.join(' => ')).join(', '), undefined !== currentPosition @@ -378,12 +407,12 @@ export class ColumnOrder { */ @cached get orderedMap(): ReadonlyMap { - return orderOf(this.args.columns(), this.map); + return orderOf(this.args.allColumns(), this.map); } @cached get orderedColumns(): Column[] { - let availableColumns = this.args.columns(); + let availableColumns = this.args.allColumns(); let availableByKey = availableColumns.reduce((keyMap, column) => { keyMap[column.key] = column; diff --git a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts index 4c37f262..fa2b974b 100644 --- a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts +++ b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts @@ -11,21 +11,28 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), }); assert.deepEqual( @@ -73,21 +80,28 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), }); assert.deepEqual( @@ -135,22 +149,29 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), save: () => {}, - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], }); assert.deepEqual( @@ -198,22 +219,29 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), save: () => {}, - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], }); assert.deepEqual( diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 14299311..98c3326d 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -509,13 +509,19 @@ module('Plugins | columnReordering', function (hooks) { assert.strictEqual(getColumnOrder(), 'B C A D', 'pre-test setup'); let order = new ColumnOrder({ - columns: () => + allColumns: () => [ { key: 'D' }, { key: 'C' }, { key: 'B' }, { key: 'A' }, ] as Column[], + availableColumns: () => ({ + A: true, + B: true, + C: true, + D: true, + }), existingOrder: new Map([ ['A', 3], ['B', 2], From e750726d94b376805e7642c89da58be63ebb488a Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 23 May 2023 15:05:24 +0100 Subject: [PATCH 05/29] chore: Fix column ordering demo --- docs/demos/external-column-ordering/demo/demo-a.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/demos/external-column-ordering/demo/demo-a.md b/docs/demos/external-column-ordering/demo/demo-a.md index 656f7e4e..95d12ce6 100644 --- a/docs/demos/external-column-ordering/demo/demo-a.md +++ b/docs/demos/external-column-ordering/demo/demo-a.md @@ -54,7 +54,11 @@ export default class extends Component { changeColumnOrder = () => { this.pendingColumnOrder = new ColumnOrder({ - columns: () => this.columns, + allColumns: () => this.columns, + availableColumns: () => this.columns.reduce(function(acc, col) { + acc[col.key] = true; + return acc; + }, {}), }); } From 7e03df03807b113031c95813b95535239d02d011 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 30 May 2023 11:46:51 +0100 Subject: [PATCH 06/29] feat: Explicitly add any missing columns to the end of the list By adding any missing columns to the end of the list whenever the data is passed _in_ we will be able to simplify any logic around reordering because it will know that it is dealing with a full set of columns --- .../src/plugins/column-reordering/plugin.ts | 31 +++++++++- .../column-reordering/rendering-test.gts | 57 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/ember-headless-table/src/plugins/column-reordering/plugin.ts b/ember-headless-table/src/plugins/column-reordering/plugin.ts index 55e1f9e7..25ca3fa3 100644 --- a/ember-headless-table/src/plugins/column-reordering/plugin.ts +++ b/ember-headless-table/src/plugins/column-reordering/plugin.ts @@ -217,8 +217,10 @@ export class ColumnOrder { } ) { if (args.existingOrder) { - this.map = new TrackedMap(args.existingOrder); - // TODO: Add anything from `allColumns` that wasn't in `existingOrder` to the end + let newOrder = new Map(args.existingOrder.entries()); + + addMissingColumnsToMap(args.allColumns(), newOrder); + this.map = new TrackedMap(newOrder); } else { this.map = new TrackedMap(args.allColumns().map((column, i) => [column.key, i])); } @@ -274,8 +276,9 @@ export class ColumnOrder { this.map.set(key, value); } + addMissingColumnsToMap(this.args.allColumns(), this.map); + this.args.save?.(map); - // TODO: Add anything from `allColumns` that wasn't in the passed `map` to the end }; /** @@ -500,3 +503,25 @@ export function orderOf( return result; } + +/** + * @private + * + * Utility to add any missing columns to the position map. By calling this whenever + * data is passed in to the system we can simplify the code within the system because + * we know we are dealing with a full set of positions. + * + * @param allColumns - A list of all columns available to the table + * @param map - A Map of `key` to position (as a zero based integer) + */ +function addMissingColumnsToMap(allColumns: Column[], map: Map): void { + if (map.size < allColumns.length) { + let maxAssignedColumn = Math.max(...map.values()); + + for (let column of allColumns) { + if (map.get(column.key) === undefined) { + map.set(column.key, ++maxAssignedColumn); + } + } + } +} diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 98c3326d..4daa1221 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -560,4 +560,61 @@ module('Plugins | columnReordering', function (hooks) { }); }); + module('with a preferences adapter where saved preferences are missing some columns', function (hooks) { + let preferences: null | PreferencesData = {}; + + class DefaultOptions extends Context { + table = headlessTable(this, { + columns: () => this.columns, + data: () => DATA, + plugins: [ColumnReordering, ColumnVisibility], + preferences: { + key: 'test-preferences', + adapter: { + persist: (_key: string, data: PreferencesData) => { + preferences = data; + }, + restore: (key: string) => { + return { + "plugins": { + "ColumnReordering": { + "columns": {}, + "table": { + "order": { + "A": 1, + "B": 0, + } + } + }, + } + }; + } + } + } + }); + } + + hooks.beforeEach(async function () { + preferences = null; + ctx = new DefaultOptions(); + setOwner(ctx, this.owner); + + await render( + // @ts-ignore + + ); + }); + + test('column order is restored from preferences', async function (assert) { + assert.strictEqual( + getColumnOrder(), + 'B A C D', + 'order declared in preferences is displayed' + ); + }); + + }); + }); From 68c30a5b132b6f9ad44390e09346844ec1b7f7db Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 30 May 2023 12:10:00 +0100 Subject: [PATCH 07/29] fix: Remove any extra columns from the passed in map This is the next piece of guaranteeing that we are dealing with a full set of positions within the addon code. By verifying on input we can save ourselves verifying on every calculation / output. --- .../src/plugins/column-reordering/plugin.ts | 39 ++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/ember-headless-table/src/plugins/column-reordering/plugin.ts b/ember-headless-table/src/plugins/column-reordering/plugin.ts index 25ca3fa3..efebd34c 100644 --- a/ember-headless-table/src/plugins/column-reordering/plugin.ts +++ b/ember-headless-table/src/plugins/column-reordering/plugin.ts @@ -216,13 +216,16 @@ export class ColumnOrder { existingOrder?: Map; } ) { + let allColumns = this.args.allColumns(); + if (args.existingOrder) { let newOrder = new Map(args.existingOrder.entries()); - addMissingColumnsToMap(args.allColumns(), newOrder); + addMissingColumnsToMap(allColumns, newOrder); + removeExtraColumnsFromMap(allColumns, newOrder); this.map = new TrackedMap(newOrder); } else { - this.map = new TrackedMap(args.allColumns().map((column, i) => [column.key, i])); + this.map = new TrackedMap(allColumns.map((column, i) => [column.key, i])); } } @@ -269,15 +272,17 @@ export class ColumnOrder { } setAll = (map: Map) => { - // TODO: Verify that the passed `map` has consectuive values set? this.map.clear(); + let allColumns = this.args.allColumns(); + + addMissingColumnsToMap(allColumns, map); + removeExtraColumnsFromMap(allColumns, map); + for (let [key, value] of map.entries()) { this.map.set(key, value); } - addMissingColumnsToMap(this.args.allColumns(), this.map); - this.args.save?.(map); }; @@ -525,3 +530,27 @@ function addMissingColumnsToMap(allColumns: Column[], map: Map): } } } + +/** + * @private + * + * Utility to remove any extra columns from the position map. By calling this whenever + * data is passed in to the system we can simplify the code within the system because + * we know we are dealing with a full set of positions. + * + * @param allColumns - A list of all columns available to the table + * @param map - A Map of `key` to position (as a zero based integer) + */ +function removeExtraColumnsFromMap(allColumns: Column[], map: Map): void { + let columnsLookup = allColumns.reduce(function (acc, { key }) { + acc[key] = true; + + return acc; + }, {} as Record); + + for (let key of map.keys()) { + if (!columnsLookup[key]) { + map.delete(key); + } + } +} From 3139ad6c8d868bfb66cddbb4ddc1ecd1d83acb3e Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 30 May 2023 12:29:48 +0100 Subject: [PATCH 08/29] test: Add a test trying to hit `removeExtraColumnsFromMap` It turns out that we can't test this functionality because we're hitting some asserts in other functionality which should now be considered unnecessary so we'll have to do some other cleaning up before this test can go green. --- .../column-reordering/rendering-test.gts | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 4daa1221..58a7d110 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -617,4 +617,64 @@ module('Plugins | columnReordering', function (hooks) { }); + module('with a preferences adapter where saved preferences have additional columns', function (hooks) { + let preferences: null | PreferencesData = {}; + + class DefaultOptions extends Context { + table = headlessTable(this, { + columns: () => this.columns, + data: () => DATA, + plugins: [ColumnReordering, ColumnVisibility], + preferences: { + key: 'test-preferences', + adapter: { + persist: (_key: string, data: PreferencesData) => { + preferences = data; + }, + restore: (key: string) => { + return { + "plugins": { + "ColumnReordering": { + "columns": {}, + "table": { + "order": { + "B": 3, + "E": 2, + "C": 1, + "D": 0, + "A": 4, + } + } + }, + } + }; + } + } + } + }); + } + + hooks.beforeEach(async function () { + preferences = null; + ctx = new DefaultOptions(); + setOwner(ctx, this.owner); + + await render( + // @ts-ignore + + ); + }); + + test('column order is restored from preferences', async function (assert) { + assert.strictEqual( + getColumnOrder(), + 'D C B A', + 'order declared in preferences is displayed' + ); + }); + + }); + }); From 41811bb15a2787c8719d629cf7e3d19ecb97f933 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 4 Jul 2023 11:26:28 +0100 Subject: [PATCH 09/29] chore: Ensure consecutive zero based positions when initialised So that we don't need to worry about checking for and filling consecutive slots throughout the code --- .../src/plugins/column-reordering/plugin.ts | 56 +++------ .../plugins/column-reordering/orderOf-test.ts | 119 +++++++----------- .../column-reordering/rendering-test.gts | 8 +- 3 files changed, 67 insertions(+), 116 deletions(-) diff --git a/ember-headless-table/src/plugins/column-reordering/plugin.ts b/ember-headless-table/src/plugins/column-reordering/plugin.ts index efebd34c..cb7bb754 100644 --- a/ember-headless-table/src/plugins/column-reordering/plugin.ts +++ b/ember-headless-table/src/plugins/column-reordering/plugin.ts @@ -272,13 +272,13 @@ export class ColumnOrder { } setAll = (map: Map) => { - this.map.clear(); - let allColumns = this.args.allColumns(); addMissingColumnsToMap(allColumns, map); removeExtraColumnsFromMap(allColumns, map); + this.map.clear(); + for (let [key, value] of map.entries()) { this.map.set(key, value); } @@ -459,54 +459,28 @@ export class ColumnOrder { * given the original (default) ordering, and then user-configurations */ export function orderOf( - columns: { key: string }[], + allColumns: { key: string }[], currentOrder: Map ): Map { - let result = new Map(); - let availableColumns = columns.map((column) => column.key); - let availableSet = new Set(availableColumns); - let current = new Map( - [...currentOrder.entries()].map(([key, position]) => [position, key]) + assert( + 'orderOf must be called with order of all columns specified', + allColumns.length === currentOrder.size && allColumns.every(({ key }) => currentOrder.has(key)) ); - /** - * O(n * log(n)) ? - */ - for (let i = 0; i < Math.max(columns.length, current.size); i++) { - let orderedKey = current.get(i); - - if (orderedKey) { - /** - * If the currentOrder specifies columns not presently available, - * ignore them - */ - if (availableSet.has(orderedKey)) { - result.set(orderedKey, i); - continue; - } - } - - let availableKey: string | undefined; - - while ((availableKey = availableColumns.shift())) { - if (result.has(availableKey) || currentOrder.has(availableKey)) { - continue; - } + // Ensure positions are consecutive and zero based + let inOrder = Array.from(currentOrder.entries()).sort( + ([_keyA, positionA], [_keyB, positionB]) => positionA - positionB + ); - break; - } + let orderedColumns = new Map(); - if (!availableKey) { - /** - * The rest of our columns likely have their order set - */ - continue; - } + let position = 0; - result.set(availableKey, i); + for (let [key] of inOrder) { + orderedColumns.set(key, position++); } - return result; + return orderedColumns; } /** diff --git a/test-app/tests/plugins/column-reordering/orderOf-test.ts b/test-app/tests/plugins/column-reordering/orderOf-test.ts index fa3171d5..3e942f24 100644 --- a/test-app/tests/plugins/column-reordering/orderOf-test.ts +++ b/test-app/tests/plugins/column-reordering/orderOf-test.ts @@ -3,118 +3,93 @@ import { module, test } from 'qunit'; import { orderOf } from 'ember-headless-table/plugins/column-reordering'; module('Plugin | column-reordering | orderOf', function () { - test('with no customizations, the original order is retained', function (assert) { + test('expected order when unchanged', function (assert) { let result = orderOf( [{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], - new Map() - ); - - assert.strictEqual(result.size, 4); - assert.deepEqual( - [...result.entries()], - [ + new Map([ ['A', 0], ['B', 1], ['C', 2], ['D', 3], - ] + ]) ); - }); - - test('with 1 custom position, columns are merged appropriately', function (assert) { - let customized = new Map([['B', 0]]); - - let result = orderOf([{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], customized); assert.strictEqual(result.size, 4); assert.deepEqual( [...result.entries()], [ - ['B', 0], - ['A', 1], + ['A', 0], + ['B', 1], ['C', 2], ['D', 3], ] ); }); - test('with middle columns moved to the outside', function (assert) { - let customized = new Map([ - ['B', 0], - ['C', 3], - ]); - - let result = orderOf([{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], customized); + test('expected order when changed', function (assert) { + let result = orderOf( + [{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], + new Map([ + ['A', 3], + ['B', 2], + ['C', 1], + ['D', 0], + ]) + ); assert.strictEqual(result.size, 4); assert.deepEqual( [...result.entries()], [ - ['B', 0], - ['A', 1], - ['D', 2], - ['C', 3], + ['D', 0], + ['C', 1], + ['B', 2], + ['A', 3], ] ); }); - test('with outer columns moved inward', function (assert) { - let customized = new Map([ - ['A', 1], - ['D', 2], - ]); - - let result = orderOf([{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], customized); - - assert.strictEqual(result.size, 4); - assert.deepEqual( - [...result.entries()], - [ - ['B', 0], + test('coerces to zero based', function (assert) { + let result = orderOf( + [{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], + new Map([ ['A', 1], - ['D', 2], + ['B', 2], ['C', 3], - ] + ['D', 4], + ]) ); - }); - test('columns specified in the customized map that do not exist are not used', function (assert) { - let customized = new Map([ - ['A', 1], - ['D', 2], - ]); - - let result = orderOf([{ key: 'A' }, { key: 'B' }, { key: 'C' }], customized); - - assert.strictEqual(result.size, 3); + assert.strictEqual(result.size, 4); assert.deepEqual( [...result.entries()], [ - ['B', 0], - ['A', 1], + ['A', 0], + ['B', 1], ['C', 2], + ['D', 3], ] ); }); - test('the first column is missing from available columns', function (assert) { - let customized = new Map([ - ['A', 1], - ['B', 0], - ['C', 2], - ['D', 3], - ]); - - let result = orderOf([{ key: 'A' }, { key: 'C' }, { key: 'D' }], customized); + test('throws with missing column', function (assert) { + assert.throws( + () => + orderOf( + [{ key: 'A' }], + new Map([ + ['A', 0], + ['B', 1], + ]) + ), + /orderOf must be called with order of all columns specified/ + ); + }); - assert.strictEqual(result.size, 3); - assert.deepEqual( - [...result.entries()], - [ - ['A', 1], - ['C', 2], - ['D', 3], - ] + test('throws with missing column', function (assert) { + assert.throws( + () => orderOf([{ key: 'A' }, { key: 'B' }], new Map([['A', 0]])), + /orderOf must be called with order of all columns specified/ ); }); }); diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 58a7d110..5b875602 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -7,7 +7,7 @@ import { on } from '@ember/modifier'; import { fn } from '@ember/helper'; import { assert, assert as debugAssert } from '@ember/debug'; import { click, findAll, render, settled } from '@ember/test-helpers'; -import { module, test } from 'qunit'; +import { module, skip, test } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; import { headlessTable } from 'ember-headless-table'; @@ -297,11 +297,12 @@ module('Plugins | columnReordering', function (hooks) { assert.strictEqual(getColumnOrder(), 'A B C D'); }); - test('without setting the order of anything, we retain the order of the columns when they are added or removed', async function (assert) { + skip('without setting the order of anything, we retain the order of the columns when they are added or removed', async function (assert) { assert.strictEqual(getColumnOrder(), 'A B C D', 'test scenario is set up'); let columnC = ctx.columns.find(column => column.key === 'C'); debugAssert('Column C is missing!', columnC); + // Is it valid to mutate the columns like this? Does something tell the plugin it has happened? ctx.columns = ctx.columns.filter(column => column !== columnC); await settled(); @@ -313,7 +314,7 @@ module('Plugins | columnReordering', function (hooks) { assert.strictEqual(getColumnOrder(), 'A B C D', 'column C is restored in the correct place'); }); - test('we can remove and add a column, and a previously set order is retained', async function (assert) { + skip('we can remove and add a column, and a previously set order is retained', async function (assert) { assert.strictEqual(getColumnOrder(), 'A B C D', 'pre-test setup'); await click('th.B .left'); @@ -323,6 +324,7 @@ module('Plugins | columnReordering', function (hooks) { let columnC = ctx.columns.find(column => column.key === 'C'); debugAssert('Column C is missing!', columnC); + // Is it valid to mutate the columns like this? Does something tell the plugin it has happened? ctx.columns = ctx.columns.filter(column => column !== columnC); await settled(); From 812c6ef2b427db297077234f76de913adbba328c Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 10:57:07 +0100 Subject: [PATCH 10/29] test: Add a failing test showing the reorder problem As described in #152, the behaviour when reordering over hidden columns is incorrect. This test illustrates the expected behaviour so that we can fix the implementation. --- .../column-reordering/rendering-test.gts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 15d023f3..db2f4554 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -443,6 +443,28 @@ module("Plugins | columnReordering", function (hooks) { "all columns are visible in the correct order", ); }); + + test('moving past hidden columns works as expected', async function (assert) { + assert.strictEqual(getColumnOrder(), 'A B C D', 'initially, columns exist as defined'); + + await click('.B.hide') + assert.strictEqual(getColumnOrder(), 'A C D', 'column B is no longer shown, and the order of the remaining columns is retained'); + + await click('th.A .right'); + assert.strictEqual(getColumnOrder(), 'C A D', 'column A was moved to the right'); + + await click('.B.show'); + assert.strictEqual(getColumnOrder(), 'B C A D', 'column B is now shown'); + + await click('.A.hide'); + assert.strictEqual(getColumnOrder(), 'B C D', 'column A is hidden'); + + await click('th.D .left'); + assert.strictEqual(getColumnOrder(), 'B D C', 'column D was moved to the left'); + + await click('.A.show'); + assert.strictEqual(getColumnOrder(), 'B D C A', 'column A has returned, and it is in the right place'); + }); }); module("with a preferences adapter", function (hooks) { From 6d1e81684029bd51a056c210b162ea2c4bb5ffa7 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 18:18:51 +0100 Subject: [PATCH 11/29] test: Update test because we can and should retain the position of hidden elements --- .../column-reordering/rendering-test.gts | 74 +++++++++++++++---- 1 file changed, 59 insertions(+), 15 deletions(-) diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index db2f4554..df8fff3c 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -375,6 +375,50 @@ module("Plugins | columnReordering", function (hooks) { "B A D C", "test scenario is set up", ); + }); + + test("a column in the middle can be moved to the right", async function (assert) { + assert.strictEqual(getColumnOrder(), "A B C D"); + + await click("th.B .right"); + + assert.strictEqual(getColumnOrder(), "A C B D"); + }); + + test("a column on the left can be moved to the right", async function (assert) { + assert.strictEqual(getColumnOrder(), "A B C D"); + + await click("th.A .right"); + + assert.strictEqual(getColumnOrder(), "B A C D"); + }); + + test("a column on the right can be moved to the left", async function (assert) { + assert.strictEqual(getColumnOrder(), "A B C D"); + + await click("th.D .left"); + + assert.strictEqual(getColumnOrder(), "A B D C"); + }); + + test("a column on the right, moved to the right, does not move", async function (assert) { + assert.strictEqual(getColumnOrder(), "A B C D"); + + await click("th.D .right"); + + assert.strictEqual(getColumnOrder(), "A B C D"); + }); + + test("a column on the left, moved to the left, does not move", async function (assert) { + assert.strictEqual(getColumnOrder(), "A B C D"); + + await click("th.A .left"); + + assert.strictEqual(getColumnOrder(), "A B C D"); + }); + + test("without setting the order of anything, we retain the order of the columns when they are added or removed", async function (assert) { + assert.strictEqual(getColumnOrder(), "A B C D", "test scenario is set up"); let columnC = ctx.columns.find((column) => column.key === "C"); debugAssert("Column C is missing!", columnC); @@ -386,7 +430,7 @@ module("Plugins | columnReordering", function (hooks) { ctx.columns = [...ctx.columns, columnC]; await settled(); - assert.strictEqual(getColumnOrder(), "B A D C", "column C is restored"); + assert.strictEqual(getColumnOrder(), "A B C D", "column C is restored in the correct place"); }); test("hiding and showing a column preserves order", async function (assert) { @@ -444,26 +488,26 @@ module("Plugins | columnReordering", function (hooks) { ); }); - test('moving past hidden columns works as expected', async function (assert) { - assert.strictEqual(getColumnOrder(), 'A B C D', 'initially, columns exist as defined'); + test("moving past hidden columns works as expected", async function (assert) { + assert.strictEqual(getColumnOrder(), "A B C D", "initially, columns exist as defined"); - await click('.B.hide') - assert.strictEqual(getColumnOrder(), 'A C D', 'column B is no longer shown, and the order of the remaining columns is retained'); + await click(".B.hide") + assert.strictEqual(getColumnOrder(), "A C D", "column B is no longer shown, and the order of the remaining columns is retained"); - await click('th.A .right'); - assert.strictEqual(getColumnOrder(), 'C A D', 'column A was moved to the right'); + await click("th.A .right"); + assert.strictEqual(getColumnOrder(), "C A D", "column A was moved to the right"); - await click('.B.show'); - assert.strictEqual(getColumnOrder(), 'B C A D', 'column B is now shown'); + await click(".B.show"); + assert.strictEqual(getColumnOrder(), "B C A D", "column B is now shown"); - await click('.A.hide'); - assert.strictEqual(getColumnOrder(), 'B C D', 'column A is hidden'); + await click(".A.hide"); + assert.strictEqual(getColumnOrder(), "B C D", "column A is hidden"); - await click('th.D .left'); - assert.strictEqual(getColumnOrder(), 'B D C', 'column D was moved to the left'); + await click("th.D .left"); + assert.strictEqual(getColumnOrder(), "B D C", "column D was moved to the left"); - await click('.A.show'); - assert.strictEqual(getColumnOrder(), 'B D C A', 'column A has returned, and it is in the right place'); + await click(".A.show"); + assert.strictEqual(getColumnOrder(), "B D C A", "column A has returned, and it is in the right place"); }); }); From a9999293f1eb344b41a42176c75f7142ce0d360c Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 18:19:46 +0100 Subject: [PATCH 12/29] demo: Add an extra column to the demo To make it easier to play with show/hiding then moving columns --- docs-app/public/docs/1-get-started/index.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs-app/public/docs/1-get-started/index.md b/docs-app/public/docs/1-get-started/index.md index 95e86545..ebccf054 100644 --- a/docs-app/public/docs/1-get-started/index.md +++ b/docs-app/public/docs/1-get-started/index.md @@ -61,6 +61,11 @@ export default class extends Component { key: "C", pluginOptions: [ColumnResizing.forColumn(() => ({ minWidth: 200 }))], }, + { + name: "column D", + key: "D", + pluginOptions: [ColumnResizing.forColumn(() => ({ minWidth: 200 }))], + }, ], data: () => this.data, plugins: [ From 9854e3bdcfffe8926ba3904f9138c20075c889d0 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Wed, 17 May 2023 18:31:47 +0100 Subject: [PATCH 13/29] feat: Implement more intuitive column sorting In order to make the previously added test pass we need to give the column reordering plugin access to _all_ columns, not just the visible ones. When moving left or right we need to iteratively swap the position of adjacent columns, continuing until we have swapped with a visible column. There are some failing tests for internals of the component on this commit but they can be fixed up later. I want to see if the new UX is desirable. --- table/src/plugins/column-reordering/plugin.ts | 106 ++++++++----- .../column-reordering/ColumnOrder-test.ts | 140 +++++++++++------- .../column-reordering/rendering-test.gts | 16 +- 3 files changed, 168 insertions(+), 94 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 446db9b1..1812d801 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -62,6 +62,7 @@ export class ColumnMeta { return this.#tableMeta.getPosition(this.column); } + // Swaps this column with the column in the new position set position(value: number) { this.#tableMeta.setPosition(this.column, value); } @@ -113,7 +114,8 @@ export class TableMeta { */ @tracked columnOrder = new ColumnOrder({ - columns: () => this.availableColumns, + allColumns: () => this.allColumns, + availableColumns: () => this.availableColumns, save: this.save, existingOrder: this.read(), }); @@ -149,7 +151,8 @@ export class TableMeta { reset() { preferences.forTable(this.table, ColumnReordering).delete('order'); this.columnOrder = new ColumnOrder({ - columns: () => this.availableColumns, + allColumns: () => this.allColumns, + availableColumns: () => this.availableColumns, save: this.save, }); } @@ -183,15 +186,23 @@ export class TableMeta { } get columns() { - return this.columnOrder.orderedColumns; + return this.columnOrder.orderedColumns.filter( + (column) => this.availableColumns[column.key], + ); } - /** - * @private - * This isn't our data to expose, but it is useful to alias - */ private get availableColumns() { - return columns.for(this.table, ColumnReordering); + return columns + .for(this.table, ColumnReordering) + .reduce>((acc, column) => { + acc[column.key] = true; + + return acc; + }, {}); + } + + private get allColumns() { + return this.table.columns.values(); } } @@ -207,13 +218,19 @@ export class ColumnOrder { constructor( private args: { - columns: () => Column[]; + allColumns: () => Column[]; + availableColumns: () => Record; save?: (order: Map) => void; existingOrder?: Map; }, ) { if (args.existingOrder) { this.map = new TrackedMap(args.existingOrder); + // TODO: Add anything from `allColumns` that wasn't in `existingOrder` to the end + } else { + this.map = new TrackedMap( + args.allColumns().map((column, i) => [column.key, i]), + ); } } @@ -226,16 +243,32 @@ export class ColumnOrder { */ @action moveLeft(key: string) { - const orderedColumns = this.orderedColumns; + if (this.map.get(key) === 0) { + return; + } let found = false; - let nextColumn: { key: string } | undefined; - for (const column of orderedColumns.reverse()) { + for (const column of this.orderedColumns.reverse()) { if (found) { - nextColumn = column; + // Shift moved column left + let currentPosition = this.map.get(key); - break; + assert('current key must exist in map', currentPosition !== undefined); + this.map.set(key, currentPosition - 1); + + // Shift displayed column right + let displayedColumnPosition = this.map.get(column.key); + + assert( + 'displaced key must exist in map', + displayedColumnPosition !== undefined, + ); + this.map.set(column.key, displayedColumnPosition + 1); + + if (this.args.availableColumns()[column.key]) { + break; + } } if (column.key === key) { @@ -243,14 +276,11 @@ export class ColumnOrder { } } - if (!nextColumn) return; - - const nextPosition = this.get(nextColumn.key); - - this.swapWith(key, nextPosition); + this.args.save?.(this.map); } setAll = (map: Map) => { + // TODO: Verify that the passed `map` has consectuive values set? this.map.clear(); for (const [key, value] of map.entries()) { @@ -258,6 +288,7 @@ export class ColumnOrder { } this.args.save?.(map); + // TODO: Add anything from `allColumns` that wasn't in the passed `map` to the end }; /** @@ -270,15 +301,28 @@ export class ColumnOrder { @action moveRight(key: string) { const orderedColumns = this.orderedColumns; - let found = false; - let nextColumn: { key: string } | undefined; for (const column of orderedColumns) { if (found) { - nextColumn = column; + // Shift moved column right + let currentPosition = this.map.get(key); - break; + assert('current key must exist in map', currentPosition !== undefined); + this.map.set(key, currentPosition + 1); + + // Shift displaced column left + let displayedColumnPosition = this.map.get(column.key); + + assert( + 'displaced key must exist in map', + displayedColumnPosition !== undefined, + ); + this.map.set(column.key, displayedColumnPosition - 1); + + if (this.args.availableColumns()[column.key]) { + break; + } } if (column.key === key) { @@ -286,11 +330,7 @@ export class ColumnOrder { } } - if (!nextColumn) return; - - const nextPosition = this.get(nextColumn.key); - - this.swapWith(key, nextPosition); + this.args.save?.(this.map); } /** @@ -320,10 +360,7 @@ export class ColumnOrder { .map((entry) => entry.join(' => ')) .join(', ') + ` and the availableColumns are: ` + - this.args - .columns() - .map((column) => column.key) - .join(', ') + + Object.keys(this.args.availableColumns()).join(', ') + ` and current "map" (${this.map.size}) is: ` + [...this.map.entries()].map((entry) => entry.join(' => ')).join(', '), undefined !== currentPosition, @@ -391,16 +428,15 @@ export class ColumnOrder { */ @cached get orderedMap(): ReadonlyMap { - return orderOf(this.args.columns(), this.map); + return orderOf(this.args.allColumns(), this.map); } @cached get orderedColumns(): Column[] { - const availableColumns = this.args.columns(); + const availableColumns = this.args.allColumns(); const availableByKey = availableColumns.reduce( (keyMap, column) => { keyMap[column.key] = column; - return keyMap; }, {} as Record, diff --git a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts index 771d7e2a..1ce1c8ef 100644 --- a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts +++ b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts @@ -11,21 +11,28 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), }); assert.deepEqual( @@ -73,21 +80,28 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), }); assert.deepEqual( @@ -135,22 +149,29 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), save: () => {}, - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], }); assert.deepEqual( @@ -198,22 +219,29 @@ module('Plugin | column-reordering | ColumnOrder', function () { let order: ColumnOrder; hooks.beforeEach(function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + { key: 'E' }, + { key: 'F' }, + /** + * This cast is a lie, but a useful one, as these #set + * tests don't actually care about the Column structure + * of this data -- only that a key exists + */ + ] as Column[]; + order = new ColumnOrder({ + allColumns: () => COLUMNS, + availableColumns: () => + COLUMNS.reduce>((acc, c) => { + acc[c.key] = true; + + return acc; + }, {}), save: () => {}, - columns: () => - [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - { key: 'D' }, - { key: 'E' }, - { key: 'F' }, - /** - * This cast is a lie, but a useful one, as these #set - * tests don't actually care about the Column structure - * of this data -- only that a key exists - */ - ] as Column[], }); assert.deepEqual( diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index df8fff3c..0058e1aa 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -630,8 +630,19 @@ module("Plugins | columnReordering", function (hooks) { assert.strictEqual(getColumnOrder(), "B C A D", "pre-test setup"); let order = new ColumnOrder({ - columns: () => - [{ key: "D" }, { key: "C" }, { key: "B" }, { key: "A" }] as Column[], + allColumns: () => + [ + { key: "D" }, + { key: "C" }, + { key: "B" }, + { key: "A" }, + ] as Column[], + availableColumns: () => ({ + A: true, + B: true, + C: true, + D: true, + }), existingOrder: new Map([ ["A", 3], ["B", 2], @@ -640,7 +651,6 @@ module("Plugins | columnReordering", function (hooks) { ]), }); - // @ts-expect-error setColumnOrder(ctx.table, order); assert.deepEqual(preferences, { From ff393d25237033570c7be6bab7d73f23687bf270 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 23 May 2023 15:05:24 +0100 Subject: [PATCH 14/29] chore: Fix column ordering demo --- docs-app/public/docs/3-demos/external-column-ordering.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs-app/public/docs/3-demos/external-column-ordering.md b/docs-app/public/docs/3-demos/external-column-ordering.md index 790d0b57..1360082a 100644 --- a/docs-app/public/docs/3-demos/external-column-ordering.md +++ b/docs-app/public/docs/3-demos/external-column-ordering.md @@ -27,7 +27,11 @@ export default class extends Component { changeColumnOrder = () => { this.pendingColumnOrder = new ColumnOrder({ - columns: () => this.columns, + allColumns: () => this.columns, + availableColumns: () => this.columns.reduce(function(acc, col) { + acc[col.key] = true; + return acc; + }, {}), }); } From a02d3ba6734e18a64f0c0fe43103bc68a57869b1 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 30 May 2023 11:46:51 +0100 Subject: [PATCH 15/29] feat: Explicitly add any missing columns to the end of the list By adding any missing columns to the end of the list whenever the data is passed _in_ we will be able to simplify any logic around reordering because it will know that it is dealing with a full set of columns --- table/src/plugins/column-reordering/plugin.ts | 34 ++++++++++- .../column-reordering/rendering-test.gts | 58 +++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 1812d801..22bec829 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -225,8 +225,10 @@ export class ColumnOrder { }, ) { if (args.existingOrder) { - this.map = new TrackedMap(args.existingOrder); - // TODO: Add anything from `allColumns` that wasn't in `existingOrder` to the end + let newOrder = new Map(args.existingOrder.entries()); + + addMissingColumnsToMap(args.allColumns(), newOrder); + this.map = new TrackedMap(newOrder); } else { this.map = new TrackedMap( args.allColumns().map((column, i) => [column.key, i]), @@ -287,8 +289,9 @@ export class ColumnOrder { this.map.set(key, value); } + addMissingColumnsToMap(this.args.allColumns(), this.map); + this.args.save?.(map); - // TODO: Add anything from `allColumns` that wasn't in the passed `map` to the end }; /** @@ -523,3 +526,28 @@ export function orderOf( return result; } + +/** + * @private + * + * Utility to add any missing columns to the position map. By calling this whenever + * data is passed in to the system we can simplify the code within the system because + * we know we are dealing with a full set of positions. + * + * @param allColumns - A list of all columns available to the table + * @param map - A Map of `key` to position (as a zero based integer) + */ +function addMissingColumnsToMap( + allColumns: Column[], + map: Map, +): void { + if (map.size < allColumns.length) { + let maxAssignedColumn = Math.max(...map.values()); + + for (let column of allColumns) { + if (map.get(column.key) === undefined) { + map.set(column.key, ++maxAssignedColumn); + } + } + } +} diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 0058e1aa..1aa33c6f 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -679,4 +679,62 @@ module("Plugins | columnReordering", function (hooks) { }); }); }); + + module('with a preferences adapter where saved preferences are missing some columns', function (hooks) { + let preferences: null | PreferencesData = {}; + + class DefaultOptions extends Context { + table = headlessTable(this, { + columns: () => this.columns, + data: () => DATA, + plugins: [ColumnReordering, ColumnVisibility], + preferences: { + key: 'test-preferences', + adapter: { + persist: (_key: string, data: PreferencesData) => { + preferences = data; + }, + restore: (key: string) => { + return { + "plugins": { + "ColumnReordering": { + "columns": {}, + "table": { + "order": { + "A": 1, + "B": 0, + } + } + }, + } + }; + } + } + } + }); + } + + hooks.beforeEach(async function () { + preferences = null; + ctx = new DefaultOptions(); + setOwner(ctx, this.owner); + + await render( + // @ts-ignore + + ); + }); + + test('column order is restored from preferences', async function (assert) { + assert.strictEqual( + getColumnOrder(), + 'B A C D', + 'order declared in preferences is displayed' + ); + }); + + }); + }); From 66e7a604ed9a9e7f72b8af899b5f9738c97ab147 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 30 May 2023 12:10:00 +0100 Subject: [PATCH 16/29] fix: Remove any extra columns from the passed in map This is the next piece of guaranteeing that we are dealing with a full set of positions within the addon code. By verifying on input we can save ourselves verifying on every calculation / output. --- table/src/plugins/column-reordering/plugin.ts | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 22bec829..69655b23 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -224,14 +224,17 @@ export class ColumnOrder { existingOrder?: Map; }, ) { + let allColumns = this.args.allColumns(); + if (args.existingOrder) { let newOrder = new Map(args.existingOrder.entries()); - addMissingColumnsToMap(args.allColumns(), newOrder); + addMissingColumnsToMap(allColumns, newOrder); + removeExtraColumnsFromMap(allColumns, newOrder); this.map = new TrackedMap(newOrder); } else { this.map = new TrackedMap( - args.allColumns().map((column, i) => [column.key, i]), + allColumns.map((column, i) => [column.key, i]), ); } } @@ -283,14 +286,17 @@ export class ColumnOrder { setAll = (map: Map) => { // TODO: Verify that the passed `map` has consectuive values set? + let allColumns = this.args.allColumns(); + + addMissingColumnsToMap(allColumns, map); + removeExtraColumnsFromMap(allColumns, map); + this.map.clear(); for (const [key, value] of map.entries()) { this.map.set(key, value); } - addMissingColumnsToMap(this.args.allColumns(), this.map); - this.args.save?.(map); }; @@ -551,3 +557,27 @@ function addMissingColumnsToMap( } } } + +/** + * @private + * + * Utility to remove any extra columns from the position map. By calling this whenever + * data is passed in to the system we can simplify the code within the system because + * we know we are dealing with a full set of positions. + * + * @param allColumns - A list of all columns available to the table + * @param map - A Map of `key` to position (as a zero based integer) + */ +function removeExtraColumnsFromMap(allColumns: Column[], map: Map): void { + let columnsLookup = allColumns.reduce(function (acc, { key }) { + acc[key] = true; + + return acc; + }, {} as Record); + + for (let key of map.keys()) { + if (!columnsLookup[key]) { + map.delete(key); + } + } +} From 53c16107ff539151c9e1b454ffb5bfdba0ca33ae Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 30 May 2023 12:29:48 +0100 Subject: [PATCH 17/29] test: Add a test trying to hit `removeExtraColumnsFromMap` It turns out that we can't test this functionality because we're hitting some asserts in other functionality which should now be considered unnecessary so we'll have to do some other cleaning up before this test can go green. --- .../column-reordering/rendering-test.gts | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 1aa33c6f..5f53d4a1 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -737,4 +737,64 @@ module("Plugins | columnReordering", function (hooks) { }); + module('with a preferences adapter where saved preferences have additional columns', function (hooks) { + let preferences: null | PreferencesData = {}; + + class DefaultOptions extends Context { + table = headlessTable(this, { + columns: () => this.columns, + data: () => DATA, + plugins: [ColumnReordering, ColumnVisibility], + preferences: { + key: 'test-preferences', + adapter: { + persist: (_key: string, data: PreferencesData) => { + preferences = data; + }, + restore: (key: string) => { + return { + "plugins": { + "ColumnReordering": { + "columns": {}, + "table": { + "order": { + "B": 3, + "E": 2, + "C": 1, + "D": 0, + "A": 4, + } + } + }, + } + }; + } + } + } + }); + } + + hooks.beforeEach(async function () { + preferences = null; + ctx = new DefaultOptions(); + setOwner(ctx, this.owner); + + await render( + // @ts-ignore + + ); + }); + + test('column order is restored from preferences', async function (assert) { + assert.strictEqual( + getColumnOrder(), + 'D C B A', + 'order declared in preferences is displayed' + ); + }); + + }); + }); From fb83848eccc52a1313fc3d485ccaf32b2d8a64b4 Mon Sep 17 00:00:00 2001 From: Kelvin Luck Date: Tue, 4 Jul 2023 11:26:28 +0100 Subject: [PATCH 18/29] chore: Ensure consecutive zero based positions when initialised So that we don't need to worry about checking for and filling consecutive slots throughout the code --- table/src/plugins/column-reordering/plugin.ts | 74 ++-- .../plugins/column-reordering/orderOf-test.ts | 122 +++---- .../column-reordering/rendering-test.gts | 315 ++++++++---------- 3 files changed, 212 insertions(+), 299 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 69655b23..3e9dd6a5 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -233,9 +233,7 @@ export class ColumnOrder { removeExtraColumnsFromMap(allColumns, newOrder); this.map = new TrackedMap(newOrder); } else { - this.map = new TrackedMap( - allColumns.map((column, i) => [column.key, i]), - ); + this.map = new TrackedMap(allColumns.map((column, i) => [column.key, i])); } } @@ -286,6 +284,7 @@ export class ColumnOrder { setAll = (map: Map) => { // TODO: Verify that the passed `map` has consectuive values set? + let allColumns = this.args.allColumns(); addMissingColumnsToMap(allColumns, map); @@ -483,54 +482,29 @@ export class ColumnOrder { * given the original (default) ordering, and then user-configurations */ export function orderOf( - columns: { key: string }[], + allColumns: { key: string }[], currentOrder: Map, ): Map { - const result = new Map(); - const availableColumns = columns.map((column) => column.key); - const availableSet = new Set(availableColumns); - const current = new Map( - [...currentOrder.entries()].map(([key, position]) => [position, key]), + assert( + 'orderOf must be called with order of all columns specified', + allColumns.length === currentOrder.size && + allColumns.every(({ key }) => currentOrder.has(key)), ); - /** - * O(n * log(n)) ? - */ - for (let i = 0; i < Math.max(columns.length, current.size); i++) { - const orderedKey = current.get(i); - - if (orderedKey) { - /** - * If the currentOrder specifies columns not presently available, - * ignore them - */ - if (availableSet.has(orderedKey)) { - result.set(orderedKey, i); - continue; - } - } - - let availableKey: string | undefined; - - while ((availableKey = availableColumns.shift())) { - if (result.has(availableKey) || currentOrder.has(availableKey)) { - continue; - } + // Ensure positions are consecutive and zero based + let inOrder = Array.from(currentOrder.entries()).sort( + ([_keyA, positionA], [_keyB, positionB]) => positionA - positionB, + ); - break; - } + let orderedColumns = new Map(); - if (!availableKey) { - /** - * The rest of our columns likely have their order set - */ - continue; - } + let position = 0; - result.set(availableKey, i); + for (let [key] of inOrder) { + orderedColumns.set(key, position++); } - return result; + return orderedColumns; } /** @@ -568,12 +542,18 @@ function addMissingColumnsToMap( * @param allColumns - A list of all columns available to the table * @param map - A Map of `key` to position (as a zero based integer) */ -function removeExtraColumnsFromMap(allColumns: Column[], map: Map): void { - let columnsLookup = allColumns.reduce(function (acc, { key }) { - acc[key] = true; +function removeExtraColumnsFromMap( + allColumns: Column[], + map: Map, +): void { + let columnsLookup = allColumns.reduce( + function (acc, { key }) { + acc[key] = true; - return acc; - }, {} as Record); + return acc; + }, + {} as Record, + ); for (let key of map.keys()) { if (!columnsLookup[key]) { diff --git a/test-app/tests/plugins/column-reordering/orderOf-test.ts b/test-app/tests/plugins/column-reordering/orderOf-test.ts index 7f48951b..51ada665 100644 --- a/test-app/tests/plugins/column-reordering/orderOf-test.ts +++ b/test-app/tests/plugins/column-reordering/orderOf-test.ts @@ -3,133 +3,93 @@ import { module, test } from 'qunit'; import { orderOf } from '@universal-ember/table/plugins/column-reordering'; module('Plugin | column-reordering | orderOf', function () { - test('with no customizations, the original order is retained', function (assert) { + test('expected order when unchanged', function (assert) { let result = orderOf( [{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], - new Map(), - ); - - assert.strictEqual(result.size, 4); - assert.deepEqual( - [...result.entries()], - [ + new Map([ ['A', 0], ['B', 1], ['C', 2], ['D', 3], - ], - ); - }); - - test('with 1 custom position, columns are merged appropriately', function (assert) { - let customized = new Map([['B', 0]]); - - let result = orderOf( - [{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], - customized, + ]), ); assert.strictEqual(result.size, 4); assert.deepEqual( [...result.entries()], [ - ['B', 0], - ['A', 1], + ['A', 0], + ['B', 1], ['C', 2], ['D', 3], ], ); }); - test('with middle columns moved to the outside', function (assert) { - let customized = new Map([ - ['B', 0], - ['C', 3], - ]); - + test('expected order when changed', function (assert) { let result = orderOf( [{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], - customized, + new Map([ + ['A', 3], + ['B', 2], + ['C', 1], + ['D', 0], + ]), ); assert.strictEqual(result.size, 4); assert.deepEqual( [...result.entries()], [ - ['B', 0], - ['A', 1], - ['D', 2], - ['C', 3], + ['D', 0], + ['C', 1], + ['B', 2], + ['A', 3], ], ); }); - test('with outer columns moved inward', function (assert) { - let customized = new Map([ - ['A', 1], - ['D', 2], - ]); - + test('coerces to zero based', function (assert) { let result = orderOf( [{ key: 'A' }, { key: 'B' }, { key: 'C' }, { key: 'D' }], - customized, - ); - - assert.strictEqual(result.size, 4); - assert.deepEqual( - [...result.entries()], - [ - ['B', 0], + new Map([ ['A', 1], - ['D', 2], + ['B', 2], ['C', 3], - ], - ); - }); - - test('columns specified in the customized map that do not exist are not used', function (assert) { - let customized = new Map([ - ['A', 1], - ['D', 2], - ]); - - let result = orderOf( - [{ key: 'A' }, { key: 'B' }, { key: 'C' }], - customized, + ['D', 4], + ]), ); - assert.strictEqual(result.size, 3); + assert.strictEqual(result.size, 4); assert.deepEqual( [...result.entries()], [ - ['B', 0], - ['A', 1], + ['A', 0], + ['B', 1], ['C', 2], + ['D', 3], ], ); }); - test('the first column is missing from available columns', function (assert) { - let customized = new Map([ - ['A', 1], - ['B', 0], - ['C', 2], - ['D', 3], - ]); - - let result = orderOf( - [{ key: 'A' }, { key: 'C' }, { key: 'D' }], - customized, + test('throws with missing column', function (assert) { + assert.throws( + () => + orderOf( + [{ key: 'A' }], + new Map([ + ['A', 0], + ['B', 1], + ]), + ), + /orderOf must be called with order of all columns specified/, ); + }); - assert.strictEqual(result.size, 3); - assert.deepEqual( - [...result.entries()], - [ - ['A', 1], - ['C', 2], - ['D', 3], - ], + test('throws with missing column', function (assert) { + assert.throws( + () => orderOf([{ key: 'A' }, { key: 'B' }], new Map([['A', 0]])), + /orderOf must be called with order of all columns specified/, ); }); }); diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 5f53d4a1..a992442d 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -5,7 +5,7 @@ import { on } from "@ember/modifier"; import { fn } from "@ember/helper"; import { assert, assert as debugAssert } from "@ember/debug"; import { click, findAll, render, settled } from "@ember/test-helpers"; -import { module, test } from "qunit"; +import { module, skip, test } from "qunit"; import { setupRenderingTest } from "ember-qunit"; import { headlessTable } from "@universal-ember/table"; @@ -340,7 +340,7 @@ module("Plugins | columnReordering", function (hooks) { assert.strictEqual(getColumnOrder(), "A B C D"); }); - test("without setting the order of anything, we cannot retain the order of the columns when they are added or removed", async function (assert) { + skip("without setting the order of anything, we retain the order of the columns when they are added or removed", async function (assert) { assert.strictEqual( getColumnOrder(), "A B C D", @@ -349,6 +349,7 @@ module("Plugins | columnReordering", function (hooks) { let columnC = ctx.columns.find((column) => column.key === "C"); debugAssert("Column C is missing!", columnC); + // Is it valid to mutate the columns like this? Does something tell the plugin it has happened? ctx.columns = ctx.columns.filter((column) => column !== columnC); await settled(); @@ -359,12 +360,12 @@ module("Plugins | columnReordering", function (hooks) { assert.strictEqual( getColumnOrder(), - "A B D C", - "column C is restored, but at the end", + "A B C D", + "column C is restored in the correct place", ); }); - test("we can remove and add a column, and a previously set order is retained", async function (assert) { + skip("we can remove and add a column, and a previously set order is retained", async function (assert) { assert.strictEqual(getColumnOrder(), "A B C D", "pre-test setup"); await click("th.B .left"); @@ -375,53 +376,10 @@ module("Plugins | columnReordering", function (hooks) { "B A D C", "test scenario is set up", ); - }); - - test("a column in the middle can be moved to the right", async function (assert) { - assert.strictEqual(getColumnOrder(), "A B C D"); - - await click("th.B .right"); - - assert.strictEqual(getColumnOrder(), "A C B D"); - }); - - test("a column on the left can be moved to the right", async function (assert) { - assert.strictEqual(getColumnOrder(), "A B C D"); - - await click("th.A .right"); - - assert.strictEqual(getColumnOrder(), "B A C D"); - }); - - test("a column on the right can be moved to the left", async function (assert) { - assert.strictEqual(getColumnOrder(), "A B C D"); - - await click("th.D .left"); - - assert.strictEqual(getColumnOrder(), "A B D C"); - }); - - test("a column on the right, moved to the right, does not move", async function (assert) { - assert.strictEqual(getColumnOrder(), "A B C D"); - - await click("th.D .right"); - - assert.strictEqual(getColumnOrder(), "A B C D"); - }); - - test("a column on the left, moved to the left, does not move", async function (assert) { - assert.strictEqual(getColumnOrder(), "A B C D"); - - await click("th.A .left"); - - assert.strictEqual(getColumnOrder(), "A B C D"); - }); - - test("without setting the order of anything, we retain the order of the columns when they are added or removed", async function (assert) { - assert.strictEqual(getColumnOrder(), "A B C D", "test scenario is set up"); let columnC = ctx.columns.find((column) => column.key === "C"); debugAssert("Column C is missing!", columnC); + // Is it valid to mutate the columns like this? Does something tell the plugin it has happened? ctx.columns = ctx.columns.filter((column) => column !== columnC); await settled(); @@ -430,7 +388,7 @@ module("Plugins | columnReordering", function (hooks) { ctx.columns = [...ctx.columns, columnC]; await settled(); - assert.strictEqual(getColumnOrder(), "A B C D", "column C is restored in the correct place"); + assert.strictEqual(getColumnOrder(), "B A D C", "column C is restored"); }); test("hiding and showing a column preserves order", async function (assert) { @@ -489,13 +447,25 @@ module("Plugins | columnReordering", function (hooks) { }); test("moving past hidden columns works as expected", async function (assert) { - assert.strictEqual(getColumnOrder(), "A B C D", "initially, columns exist as defined"); + assert.strictEqual( + getColumnOrder(), + "A B C D", + "initially, columns exist as defined", + ); - await click(".B.hide") - assert.strictEqual(getColumnOrder(), "A C D", "column B is no longer shown, and the order of the remaining columns is retained"); + await click(".B.hide"); + assert.strictEqual( + getColumnOrder(), + "A C D", + "column B is no longer shown, and the order of the remaining columns is retained", + ); await click("th.A .right"); - assert.strictEqual(getColumnOrder(), "C A D", "column A was moved to the right"); + assert.strictEqual( + getColumnOrder(), + "C A D", + "column A was moved to the right", + ); await click(".B.show"); assert.strictEqual(getColumnOrder(), "B C A D", "column B is now shown"); @@ -504,10 +474,18 @@ module("Plugins | columnReordering", function (hooks) { assert.strictEqual(getColumnOrder(), "B C D", "column A is hidden"); await click("th.D .left"); - assert.strictEqual(getColumnOrder(), "B D C", "column D was moved to the left"); + assert.strictEqual( + getColumnOrder(), + "B D C", + "column D was moved to the left", + ); await click(".A.show"); - assert.strictEqual(getColumnOrder(), "B D C A", "column A has returned, and it is in the right place"); + assert.strictEqual( + getColumnOrder(), + "B D C A", + "column A has returned, and it is in the right place", + ); }); }); @@ -631,12 +609,7 @@ module("Plugins | columnReordering", function (hooks) { let order = new ColumnOrder({ allColumns: () => - [ - { key: "D" }, - { key: "C" }, - { key: "B" }, - { key: "A" }, - ] as Column[], + [{ key: "D" }, { key: "C" }, { key: "B" }, { key: "A" }] as Column[], availableColumns: () => ({ A: true, B: true, @@ -651,6 +624,7 @@ module("Plugins | columnReordering", function (hooks) { ]), }); + // @ts-expect-error setColumnOrder(ctx.table, order); assert.deepEqual(preferences, { @@ -680,121 +654,120 @@ module("Plugins | columnReordering", function (hooks) { }); }); - module('with a preferences adapter where saved preferences are missing some columns', function (hooks) { - let preferences: null | PreferencesData = {}; - - class DefaultOptions extends Context { - table = headlessTable(this, { - columns: () => this.columns, - data: () => DATA, - plugins: [ColumnReordering, ColumnVisibility], - preferences: { - key: 'test-preferences', - adapter: { - persist: (_key: string, data: PreferencesData) => { - preferences = data; - }, - restore: (key: string) => { - return { - "plugins": { - "ColumnReordering": { - "columns": {}, - "table": { - "order": { - "A": 1, - "B": 0, - } - } + module( + "with a preferences adapter where saved preferences are missing some columns", + function (hooks) { + let preferences: null | PreferencesData = {}; + + class DefaultOptions extends Context { + table = headlessTable(this, { + columns: () => this.columns, + data: () => DATA, + plugins: [ColumnReordering, ColumnVisibility], + preferences: { + key: "test-preferences", + adapter: { + persist: (_key: string, data: PreferencesData) => { + preferences = data; + }, + restore: (key: string) => { + return { + plugins: { + ColumnReordering: { + columns: {}, + table: { + order: { + A: 1, + B: 0, + }, + }, + }, }, - } - }; - } - } - } - }); - } - - hooks.beforeEach(async function () { - preferences = null; - ctx = new DefaultOptions(); - setOwner(ctx, this.owner); - - await render( - // @ts-ignore - - ); - }); - - test('column order is restored from preferences', async function (assert) { - assert.strictEqual( - getColumnOrder(), - 'B A C D', - 'order declared in preferences is displayed' - ); - }); + }; + }, + }, + }, + }); + } - }); + hooks.beforeEach(async function () { + preferences = null; + ctx = new DefaultOptions(); + setOwner(ctx, this.owner); - module('with a preferences adapter where saved preferences have additional columns', function (hooks) { - let preferences: null | PreferencesData = {}; - - class DefaultOptions extends Context { - table = headlessTable(this, { - columns: () => this.columns, - data: () => DATA, - plugins: [ColumnReordering, ColumnVisibility], - preferences: { - key: 'test-preferences', - adapter: { - persist: (_key: string, data: PreferencesData) => { - preferences = data; - }, - restore: (key: string) => { - return { - "plugins": { - "ColumnReordering": { - "columns": {}, - "table": { - "order": { - "B": 3, - "E": 2, - "C": 1, - "D": 0, - "A": 4, - } - } - }, - } - }; - } - } - } + await render( + // @ts-ignore + , + ); }); - } - hooks.beforeEach(async function () { - preferences = null; - ctx = new DefaultOptions(); - setOwner(ctx, this.owner); - - await render( - // @ts-ignore - - ); - }); + test("column order is restored from preferences", async function (assert) { + assert.strictEqual( + getColumnOrder(), + "B A C D", + "order declared in preferences is displayed", + ); + }); + }, + ); + + module( + "with a preferences adapter where saved preferences have additional columns", + function (hooks) { + let preferences: null | PreferencesData = {}; + + class DefaultOptions extends Context { + table = headlessTable(this, { + columns: () => this.columns, + data: () => DATA, + plugins: [ColumnReordering, ColumnVisibility], + preferences: { + key: "test-preferences", + adapter: { + persist: (_key: string, data: PreferencesData) => { + preferences = data; + }, + restore: (key: string) => { + return { + plugins: { + ColumnReordering: { + columns: {}, + table: { + order: { + B: 3, + E: 2, + C: 1, + D: 0, + A: 4, + }, + }, + }, + }, + }; + }, + }, + }, + }); + } - test('column order is restored from preferences', async function (assert) { - assert.strictEqual( - getColumnOrder(), - 'D C B A', - 'order declared in preferences is displayed' - ); - }); + hooks.beforeEach(async function () { + preferences = null; + ctx = new DefaultOptions(); + setOwner(ctx, this.owner); - }); + await render( + // @ts-ignore + , + ); + }); + test("column order is restored from preferences", async function (assert) { + assert.strictEqual( + getColumnOrder(), + "D C B A", + "order declared in preferences is displayed", + ); + }); + }, + ); }); From 1d24d8d843c289901f4adf284e0c82caa1a7d29f Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 11 Apr 2025 16:29:05 +0200 Subject: [PATCH 19/29] cleanup --- table/src/plugins/column-reordering/plugin.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 3e9dd6a5..cd5cb78a 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -246,13 +246,14 @@ export class ColumnOrder { */ @action moveLeft(key: string) { + const orderedColumns = this.orderedColumns; if (this.map.get(key) === 0) { return; } let found = false; - for (const column of this.orderedColumns.reverse()) { + for (const column of orderedColumns.reverse()) { if (found) { // Shift moved column left let currentPosition = this.map.get(key); From a87ffcefe278df60958298fdbf12a83fc1921f01 Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 11 Apr 2025 16:30:27 +0200 Subject: [PATCH 20/29] cleanuo --- table/src/plugins/column-reordering/plugin.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index cd5cb78a..9e95e446 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -284,8 +284,6 @@ export class ColumnOrder { } setAll = (map: Map) => { - // TODO: Verify that the passed `map` has consectuive values set? - let allColumns = this.args.allColumns(); addMissingColumnsToMap(allColumns, map); From c8c709155e212b75017baee6b7d8121df661c5cf Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 13:09:00 +0200 Subject: [PATCH 21/29] Fix column reordering crash when column set changes between deployments. Graceful handling of column mismatches. When localStorage contains a column order from a previous deployment with different columns, the function now normalizes the map by adding missing columns and removing extra ones instead of throwing an error. --- table/src/plugins/column-reordering/plugin.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index da376f9e..515e4da7 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -487,14 +487,15 @@ export function orderOf( allColumns: { key: string }[], currentOrder: Map, ): Map { - assert( - 'orderOf must be called with order of all columns specified', - allColumns.length === currentOrder.size && - allColumns.every(({ key }) => currentOrder.has(key)), - ); + // Create a copy of the order map to avoid mutating the input + let workingOrder = new Map(currentOrder); + + // Handle mismatches gracefully by normalizing the map + addMissingColumnsToMap(allColumns, workingOrder); + removeExtraColumnsFromMap(allColumns, workingOrder); // Ensure positions are consecutive and zero based - let inOrder = Array.from(currentOrder.entries()).sort( + let inOrder = Array.from(workingOrder.entries()).sort( ([_keyA, positionA], [_keyB, positionB]) => positionA - positionB, ); From df4499387b6c21f9e8fab8c4f04c8eec8d86e8bb Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 13:12:54 +0200 Subject: [PATCH 22/29] review comment --- table/src/plugins/column-reordering/plugin.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 515e4da7..101b2e1d 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -194,6 +194,10 @@ export class TableMeta { ); } + /** + * @private + * This isn't our data to expose, but it is useful to alias + */ private get availableColumns() { return columns .for(this.table, ColumnReordering) From ff21ce208f437aa5fb58e1a121a0e0212e3640d7 Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 13:32:21 +0200 Subject: [PATCH 23/29] address review comments, + be more resilient towards missing/added columns --- .../docs/2-plugins/column-visibility.md | 91 +++++++++++++++++++ .../docs/3-demos/external-column-ordering.md | 12 ++- table/src/plugins/column-reordering/plugin.ts | 51 +++++++++-- .../plugins/column-reordering/orderOf-test.ts | 36 +++++--- 4 files changed, 165 insertions(+), 25 deletions(-) diff --git a/docs-app/public/docs/2-plugins/column-visibility.md b/docs-app/public/docs/2-plugins/column-visibility.md index cf8a37a9..b039bf15 100644 --- a/docs-app/public/docs/2-plugins/column-visibility.md +++ b/docs-app/public/docs/2-plugins/column-visibility.md @@ -127,3 +127,94 @@ but the important things to make sure exist are: - buttons are focusable - buttons can be navigated to and pressed via keyboard - buttons can be navigated to and pressed via screen reader tool + +## Combining with Column Reordering + +When using both `ColumnVisibility` and `ColumnReordering` together, the reordering automatically skips over hidden columns when moving left/right. + + diff --git a/docs-app/public/docs/3-demos/external-column-ordering.md b/docs-app/public/docs/3-demos/external-column-ordering.md index 1360082a..ff0b74c4 100644 --- a/docs-app/public/docs/3-demos/external-column-ordering.md +++ b/docs-app/public/docs/3-demos/external-column-ordering.md @@ -26,12 +26,16 @@ export default class extends Component { @tracked pendingColumnOrder; changeColumnOrder = () => { + // ColumnOrder takes allColumns to track the ordering. + // availableColumns (optional) is used when combining with ColumnVisibility + // to skip over hidden columns during reordering. this.pendingColumnOrder = new ColumnOrder({ allColumns: () => this.columns, - availableColumns: () => this.columns.reduce(function(acc, col) { - acc[col.key] = true; - return acc; - }, {}), + // If using ColumnVisibility plugin, provide availableColumns to skip hidden columns: + // availableColumns: () => this.columns.reduce((acc, col) => { + // acc[col.key] = meta(col).ColumnVisibility?.isVisible !== false; + // return acc; + // }, {}), }); } diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 101b2e1d..16c945ad 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -225,9 +225,32 @@ export class ColumnOrder { constructor( private args: { + /** + * All columns in the table (including hidden ones if using ColumnVisibility). + * Used to track the complete ordering of all columns. + */ allColumns: () => Column[]; - availableColumns: () => Record; + /** + * Optional: Record of which columns are currently visible/available. + * When provided, moveLeft/moveRight will skip over hidden columns. + * If not provided, all columns are assumed to be available. + * + * Example when using ColumnVisibility: + * ```ts + * availableColumns: () => columns.reduce((acc, col) => { + * acc[col.key] = meta(col).ColumnVisibility?.isVisible !== false; + * return acc; + * }, {}) + * ``` + */ + availableColumns?: () => Record; + /** + * Optional: Callback to persist the column order (e.g., to localStorage). + */ save?: (order: Map) => void; + /** + * Optional: Previously saved column order to restore. + */ existingOrder?: Map; }, ) { @@ -244,6 +267,22 @@ export class ColumnOrder { } } + /** + * @private + * Helper to get available columns, defaulting to all columns if not specified + */ + private getAvailableColumns(): Record { + if (this.args.availableColumns) { + return this.args.availableColumns(); + } + + // Default: all columns are available + return this.args.allColumns().reduce((acc, col) => { + acc[col.key] = true; + return acc; + }, {} as Record); + } + /** * To account for columnVisibilty, we need to: * - get the list of visible columns @@ -277,7 +316,7 @@ export class ColumnOrder { ); this.map.set(column.key, displayedColumnPosition + 1); - if (this.args.availableColumns()[column.key]) { + if (this.getAvailableColumns()[column.key]) { break; } } @@ -334,7 +373,7 @@ export class ColumnOrder { ); this.map.set(column.key, displayedColumnPosition - 1); - if (this.args.availableColumns()[column.key]) { + if (this.getAvailableColumns()[column.key]) { break; } } @@ -374,7 +413,7 @@ export class ColumnOrder { .map((entry) => entry.join(' => ')) .join(', ') + ` and the availableColumns are: ` + - Object.keys(this.args.availableColumns()).join(', ') + + Object.keys(this.getAvailableColumns()).join(', ') + ` and current "map" (${this.map.size}) is: ` + [...this.map.entries()].map((entry) => entry.join(' => ')).join(', '), undefined !== currentPosition, @@ -525,7 +564,7 @@ export function orderOf( * @param map - A Map of `key` to position (as a zero based integer) */ function addMissingColumnsToMap( - allColumns: Column[], + allColumns: { key: string }[], map: Map, ): void { if (map.size < allColumns.length) { @@ -550,7 +589,7 @@ function addMissingColumnsToMap( * @param map - A Map of `key` to position (as a zero based integer) */ function removeExtraColumnsFromMap( - allColumns: Column[], + allColumns: { key: string }[], map: Map, ): void { let columnsLookup = allColumns.reduce( diff --git a/test-app/tests/plugins/column-reordering/orderOf-test.ts b/test-app/tests/plugins/column-reordering/orderOf-test.ts index 51ada665..deb4d8ae 100644 --- a/test-app/tests/plugins/column-reordering/orderOf-test.ts +++ b/test-app/tests/plugins/column-reordering/orderOf-test.ts @@ -72,24 +72,30 @@ module('Plugin | column-reordering | orderOf', function () { ); }); - test('throws with missing column', function (assert) { - assert.throws( - () => - orderOf( - [{ key: 'A' }], - new Map([ - ['A', 0], - ['B', 1], - ]), - ), - /orderOf must be called with order of all columns specified/, + test('handles extra columns in map (removes them)', function (assert) { + let result = orderOf( + [{ key: 'A' }], + new Map([ + ['A', 0], + ['B', 1], + ]), ); + + assert.strictEqual(result.size, 1, 'only has the available column'); + assert.deepEqual([...result.entries()], [['A', 0]], 'column B was removed'); }); - test('throws with missing column', function (assert) { - assert.throws( - () => orderOf([{ key: 'A' }, { key: 'B' }], new Map([['A', 0]])), - /orderOf must be called with order of all columns specified/, + test('handles missing columns in map (adds them)', function (assert) { + let result = orderOf([{ key: 'A' }, { key: 'B' }], new Map([['A', 0]])); + + assert.strictEqual(result.size, 2, 'has both columns'); + assert.deepEqual( + [...result.entries()], + [ + ['A', 0], + ['B', 1], + ], + 'column B was added at the end', ); }); }); From ce1d44e72af56e719d2cd9b2fc4798a26c5bb3f7 Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 13:53:27 +0200 Subject: [PATCH 24/29] // Add any missing columns to the end // DON'T remove extra columns - they might be hidden columns, not deleted ones --- table/src/plugins/column-reordering/plugin.ts | 13 +++++++++---- .../tests/plugins/column-reordering/orderOf-test.ts | 6 +++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index 16c945ad..a14ae094 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -524,18 +524,23 @@ export class ColumnOrder { * @private * * Utility for helping determine the percieved order of a set of columns - * given the original (default) ordering, and then user-configurations + * given the original (default) ordering, and then user-configurations. + * + * This function adds missing columns but preserves extra columns in the map + * (they might be hidden, not deleted). */ export function orderOf( allColumns: { key: string }[], currentOrder: Map, ): Map { - // Create a copy of the order map to avoid mutating the input + // Create a copy to avoid mutating the input let workingOrder = new Map(currentOrder); - // Handle mismatches gracefully by normalizing the map + // Add any missing columns to the end addMissingColumnsToMap(allColumns, workingOrder); - removeExtraColumnsFromMap(allColumns, workingOrder); + + // DON'T remove extra columns - they might be hidden columns, not deleted ones + // The ColumnOrder constructor handles removal of truly deleted columns // Ensure positions are consecutive and zero based let inOrder = Array.from(workingOrder.entries()).sort( diff --git a/test-app/tests/plugins/column-reordering/orderOf-test.ts b/test-app/tests/plugins/column-reordering/orderOf-test.ts index deb4d8ae..959259d4 100644 --- a/test-app/tests/plugins/column-reordering/orderOf-test.ts +++ b/test-app/tests/plugins/column-reordering/orderOf-test.ts @@ -72,7 +72,7 @@ module('Plugin | column-reordering | orderOf', function () { ); }); - test('handles extra columns in map (removes them)', function (assert) { + test('handles extra columns in map (preserves them for hidden columns)', function (assert) { let result = orderOf( [{ key: 'A' }], new Map([ @@ -81,8 +81,8 @@ module('Plugin | column-reordering | orderOf', function () { ]), ); - assert.strictEqual(result.size, 1, 'only has the available column'); - assert.deepEqual([...result.entries()], [['A', 0]], 'column B was removed'); + assert.strictEqual(result.size, 2, 'preserves all columns including hidden ones'); + assert.deepEqual([...result.entries()], [['A', 0], ['B', 1]], 'column B was preserved (might be hidden)'); }); test('handles missing columns in map (adds them)', function (assert) { From 8c36696b81e92ac71f790f03f45da8c51ea4404c Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 14:09:49 +0200 Subject: [PATCH 25/29] lint --- .../plugins/column-reordering/orderOf-test.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test-app/tests/plugins/column-reordering/orderOf-test.ts b/test-app/tests/plugins/column-reordering/orderOf-test.ts index 959259d4..9f6a64e1 100644 --- a/test-app/tests/plugins/column-reordering/orderOf-test.ts +++ b/test-app/tests/plugins/column-reordering/orderOf-test.ts @@ -81,8 +81,19 @@ module('Plugin | column-reordering | orderOf', function () { ]), ); - assert.strictEqual(result.size, 2, 'preserves all columns including hidden ones'); - assert.deepEqual([...result.entries()], [['A', 0], ['B', 1]], 'column B was preserved (might be hidden)'); + assert.strictEqual( + result.size, + 2, + 'preserves all columns including hidden ones', + ); + assert.deepEqual( + [...result.entries()], + [ + ['A', 0], + ['B', 1], + ], + 'column B was preserved (might be hidden)', + ); }); test('handles missing columns in map (adds them)', function (assert) { From feb5a9ff9dd248ed63da46268c2dd8ad5aea45bf Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 15:41:09 +0200 Subject: [PATCH 26/29] revert api of ColumnOrder to be non-breaking, and rename availableColumns to visibleColumns to align better with naming elsewhere in @universal-ember/table --- .../docs/3-demos/external-column-ordering.md | 22 +-- table/src/plugins/column-reordering/plugin.ts | 94 +++++------ .../column-reordering/ColumnOrder-test.ts | 146 +++++++++++++++++- .../column-reordering/rendering-test.gts | 4 +- 4 files changed, 204 insertions(+), 62 deletions(-) diff --git a/docs-app/public/docs/3-demos/external-column-ordering.md b/docs-app/public/docs/3-demos/external-column-ordering.md index ff0b74c4..fb0791bf 100644 --- a/docs-app/public/docs/3-demos/external-column-ordering.md +++ b/docs-app/public/docs/3-demos/external-column-ordering.md @@ -26,17 +26,21 @@ export default class extends Component { @tracked pendingColumnOrder; changeColumnOrder = () => { - // ColumnOrder takes allColumns to track the ordering. - // availableColumns (optional) is used when combining with ColumnVisibility - // to skip over hidden columns during reordering. + // Basic usage (backwards compatible): + // Pass your columns and they're all treated as visible this.pendingColumnOrder = new ColumnOrder({ - allColumns: () => this.columns, - // If using ColumnVisibility plugin, provide availableColumns to skip hidden columns: - // availableColumns: () => this.columns.reduce((acc, col) => { - // acc[col.key] = meta(col).ColumnVisibility?.isVisible !== false; - // return acc; - // }, {}), + columns: () => this.columns, }); + + // Advanced usage (with ColumnVisibility plugin): + // Pass ALL columns and provide a visibleColumns map + // this.pendingColumnOrder = new ColumnOrder({ + // columns: () => this.table.columns.values(), // All columns (including hidden) + // visibleColumns: () => this.columns.reduce((acc, col) => { + // acc[col.key] = meta(col).ColumnVisibility?.isVisible !== false; + // return acc; + // }, {}), + // }); } handleReconfigure = () => { diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index a14ae094..c3103442 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -114,8 +114,8 @@ export class TableMeta { */ @tracked columnOrder = new ColumnOrder({ - allColumns: () => this.allColumns, - availableColumns: () => this.availableColumns, + columns: () => this.allColumns, + visibleColumns: () => this.visibleColumns, save: this.save, existingOrder: this.read(), }); @@ -154,8 +154,8 @@ export class TableMeta { reset() { preferences.forTable(this.table, ColumnReordering).delete('order'); this.columnOrder = new ColumnOrder({ - allColumns: () => this.allColumns, - availableColumns: () => this.availableColumns, + columns: () => this.allColumns, + visibleColumns: () => this.visibleColumns, save: this.save, }); } @@ -190,7 +190,7 @@ export class TableMeta { get columns() { return this.columnOrder.orderedColumns.filter( - (column) => this.availableColumns[column.key], + (column) => this.visibleColumns[column.key], ); } @@ -198,7 +198,7 @@ export class TableMeta { * @private * This isn't our data to expose, but it is useful to alias */ - private get availableColumns() { + private get visibleColumns() { return columns .for(this.table, ColumnReordering) .reduce>((acc, column) => { @@ -226,24 +226,32 @@ export class ColumnOrder { constructor( private args: { /** - * All columns in the table (including hidden ones if using ColumnVisibility). - * Used to track the complete ordering of all columns. + * All columns to track in the ordering. + * + * Backwards compatible usage (without ColumnVisibility): + * - Pass only the columns you want to display + * - All columns are treated as visible + * + * New usage (with ColumnVisibility): + * - Pass ALL columns (including hidden ones) + * - Provide `visibleColumns` to indicate which are visible + * - Hidden columns maintain their position when toggled */ - allColumns: () => Column[]; + columns: () => Column[]; /** - * Optional: Record of which columns are currently visible/available. + * Optional: Record of which columns are currently visible. * When provided, moveLeft/moveRight will skip over hidden columns. - * If not provided, all columns are assumed to be available. + * When omitted, all columns from `columns` are treated as visible (backwards compatible). * * Example when using ColumnVisibility: * ```ts - * availableColumns: () => columns.reduce((acc, col) => { + * visibleColumns: () => columns.reduce((acc, col) => { * acc[col.key] = meta(col).ColumnVisibility?.isVisible !== false; * return acc; * }, {}) * ``` */ - availableColumns?: () => Record; + visibleColumns?: () => Record; /** * Optional: Callback to persist the column order (e.g., to localStorage). */ @@ -254,7 +262,7 @@ export class ColumnOrder { existingOrder?: Map; }, ) { - let allColumns = this.args.allColumns(); + let allColumns = this.args.columns(); if (args.existingOrder) { let newOrder = new Map(args.existingOrder.entries()); @@ -269,15 +277,15 @@ export class ColumnOrder { /** * @private - * Helper to get available columns, defaulting to all columns if not specified + * Helper to get visible columns, defaulting to all columns if not specified */ - private getAvailableColumns(): Record { - if (this.args.availableColumns) { - return this.args.availableColumns(); + private getVisibleColumns(): Record { + if (this.args.visibleColumns) { + return this.args.visibleColumns(); } - // Default: all columns are available - return this.args.allColumns().reduce((acc, col) => { + // Default: all columns are visible + return this.args.columns().reduce((acc, col) => { acc[col.key] = true; return acc; }, {} as Record); @@ -316,7 +324,7 @@ export class ColumnOrder { ); this.map.set(column.key, displayedColumnPosition + 1); - if (this.getAvailableColumns()[column.key]) { + if (this.getVisibleColumns()[column.key]) { break; } } @@ -330,7 +338,7 @@ export class ColumnOrder { } setAll = (map: Map) => { - let allColumns = this.args.allColumns(); + let allColumns = this.args.columns(); addMissingColumnsToMap(allColumns, map); removeExtraColumnsFromMap(allColumns, map); @@ -373,7 +381,7 @@ export class ColumnOrder { ); this.map.set(column.key, displayedColumnPosition - 1); - if (this.getAvailableColumns()[column.key]) { + if (this.getVisibleColumns()[column.key]) { break; } } @@ -412,8 +420,8 @@ export class ColumnOrder { [...this.orderedMap.entries()] .map((entry) => entry.join(' => ')) .join(', ') + - ` and the availableColumns are: ` + - Object.keys(this.getAvailableColumns()).join(', ') + + ` and the visibleColumns are: ` + + Object.keys(this.getVisibleColumns()).join(', ') + ` and current "map" (${this.map.size}) is: ` + [...this.map.entries()].map((entry) => entry.join(' => ')).join(', '), undefined !== currentPosition, @@ -481,25 +489,25 @@ export class ColumnOrder { */ @cached get orderedMap(): ReadonlyMap { - return orderOf(this.args.allColumns(), this.map); + return orderOf(this.args.columns(), this.map); } @cached get orderedColumns(): Column[] { - const availableColumns = this.args.allColumns(); - const availableByKey = availableColumns.reduce( + const allColumns = this.args.columns(); + const columnsByKey = allColumns.reduce( (keyMap, column) => { keyMap[column.key] = column; return keyMap; }, {} as Record, ); - const mergedOrder = orderOf(availableColumns, this.map); + const mergedOrder = orderOf(allColumns, this.map); - const result: Column[] = Array.from({ length: availableColumns.length }); + const result: Column[] = Array.from({ length: allColumns.length }); for (const [key, position] of mergedOrder.entries()) { - const column = availableByKey[key]; + const column = columnsByKey[key]; assert(`Could not find column for pair: ${key} @ @{position}`, column); result[position] = column; @@ -507,13 +515,13 @@ export class ColumnOrder { assert( `Generated orderedColumns' length (${result.filter(Boolean).length}) ` + - `does not match the length of available columns (${availableColumns.length}). ` + + `does not match the length of all columns (${allColumns.length}). ` + `orderedColumns: ${result .filter(Boolean) .map((c) => c.key) .join(', ')} -- ` + - `available columns: ${availableColumns.map((c) => c.key).join(', ')}`, - result.filter(Boolean).length === availableColumns.length, + `all columns: ${allColumns.map((c) => c.key).join(', ')}`, + result.filter(Boolean).length === allColumns.length, ); return result.filter(Boolean); @@ -530,14 +538,14 @@ export class ColumnOrder { * (they might be hidden, not deleted). */ export function orderOf( - allColumns: { key: string }[], + columns: { key: string }[], currentOrder: Map, ): Map { // Create a copy to avoid mutating the input let workingOrder = new Map(currentOrder); // Add any missing columns to the end - addMissingColumnsToMap(allColumns, workingOrder); + addMissingColumnsToMap(columns, workingOrder); // DON'T remove extra columns - they might be hidden columns, not deleted ones // The ColumnOrder constructor handles removal of truly deleted columns @@ -565,17 +573,17 @@ export function orderOf( * data is passed in to the system we can simplify the code within the system because * we know we are dealing with a full set of positions. * - * @param allColumns - A list of all columns available to the table + * @param columns - A list of all columns available to the table * @param map - A Map of `key` to position (as a zero based integer) */ function addMissingColumnsToMap( - allColumns: { key: string }[], + columns: { key: string }[], map: Map, ): void { - if (map.size < allColumns.length) { + if (map.size < columns.length) { let maxAssignedColumn = Math.max(...map.values()); - for (let column of allColumns) { + for (let column of columns) { if (map.get(column.key) === undefined) { map.set(column.key, ++maxAssignedColumn); } @@ -590,14 +598,14 @@ function addMissingColumnsToMap( * data is passed in to the system we can simplify the code within the system because * we know we are dealing with a full set of positions. * - * @param allColumns - A list of all columns available to the table + * @param columns - A list of all columns available to the table * @param map - A Map of `key` to position (as a zero based integer) */ function removeExtraColumnsFromMap( - allColumns: { key: string }[], + columns: { key: string }[], map: Map, ): void { - let columnsLookup = allColumns.reduce( + let columnsLookup = columns.reduce( function (acc, { key }) { acc[key] = true; diff --git a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts index 1ce1c8ef..59a693ba 100644 --- a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts +++ b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts @@ -26,8 +26,8 @@ module('Plugin | column-reordering | ColumnOrder', function () { ] as Column[]; order = new ColumnOrder({ - allColumns: () => COLUMNS, - availableColumns: () => + columns: () => COLUMNS, + visibleColumns: () => COLUMNS.reduce>((acc, c) => { acc[c.key] = true; @@ -95,8 +95,8 @@ module('Plugin | column-reordering | ColumnOrder', function () { ] as Column[]; order = new ColumnOrder({ - allColumns: () => COLUMNS, - availableColumns: () => + columns: () => COLUMNS, + visibleColumns: () => COLUMNS.reduce>((acc, c) => { acc[c.key] = true; @@ -164,8 +164,8 @@ module('Plugin | column-reordering | ColumnOrder', function () { ] as Column[]; order = new ColumnOrder({ - allColumns: () => COLUMNS, - availableColumns: () => + columns: () => COLUMNS, + visibleColumns: () => COLUMNS.reduce>((acc, c) => { acc[c.key] = true; @@ -234,8 +234,8 @@ module('Plugin | column-reordering | ColumnOrder', function () { ] as Column[]; order = new ColumnOrder({ - allColumns: () => COLUMNS, - availableColumns: () => + columns: () => COLUMNS, + visibleColumns: () => COLUMNS.reduce>((acc, c) => { acc[c.key] = true; @@ -361,4 +361,134 @@ module('Plugin | column-reordering | ColumnOrder', function () { }); }); }); + + module('Backwards compatibility', function () { + test('without visibleColumns parameter, all columns are treated as visible', function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + ] as Column[]; + + // Old usage - no visibleColumns parameter + const order = new ColumnOrder({ + columns: () => COLUMNS, + }); + + assert.deepEqual( + toEntries(order.orderedMap), + [ + ['A', 0], + ['B', 1], + ['C', 2], + ], + 'columns are in default order', + ); + + // moveRight should work normally + order.moveRight('A'); + + assert.deepEqual( + toEntries(order.orderedMap), + [ + ['B', 0], + ['A', 1], + ['C', 2], + ], + 'column A moved right', + ); + }); + + test('with visibleColumns parameter, hidden columns are tracked but skipped', function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + ] as Column[]; + + // New usage - with visibleColumns (C is hidden) + const order = new ColumnOrder({ + columns: () => COLUMNS, + visibleColumns: () => ({ + A: true, + B: true, + C: false, // hidden + D: true, + }), + }); + + assert.deepEqual( + toEntries(order.orderedMap), + [ + ['A', 0], + ['B', 1], + ['C', 2], + ['D', 3], + ], + 'all columns are tracked in order', + ); + + // moveRight on A should skip hidden C and land on D + order.moveRight('A'); + + assert.deepEqual( + toEntries(order.orderedMap), + [ + ['B', 0], + ['A', 1], + ['C', 2], + ['D', 3], + ], + 'A moved to B position, B moved left', + ); + + // Move A right again - should swap with C, then continue to D + order.moveRight('A'); + + assert.deepEqual( + toEntries(order.orderedMap), + [ + ['B', 0], + ['C', 1], + ['D', 2], + ['A', 3], + ], + 'A swapped with hidden C (1->2), then swapped with visible D (2->3), ending at position 3', + ); + }); + + test('hidden columns maintain position when columns are reordered', function (assert) { + const COLUMNS = [ + { key: 'A' }, + { key: 'B' }, + { key: 'C' }, + { key: 'D' }, + ] as Column[]; + + const order = new ColumnOrder({ + columns: () => COLUMNS, + visibleColumns: () => ({ + A: true, + B: false, // hidden + C: true, + D: true, + }), + }); + + // Move D to the left (should skip hidden B) + order.moveLeft('D'); + + assert.deepEqual( + toEntries(order.orderedMap), + [ + ['A', 0], + ['B', 1], // B stays at position 1 (hidden) + ['D', 2], // D moved from 3 to 2 + ['C', 3], // C moved from 2 to 3 + ], + 'hidden column B maintained its position', + ); + }); + }); }); diff --git a/test-app/tests/plugins/column-reordering/rendering-test.gts b/test-app/tests/plugins/column-reordering/rendering-test.gts index 941353f3..78b0a617 100644 --- a/test-app/tests/plugins/column-reordering/rendering-test.gts +++ b/test-app/tests/plugins/column-reordering/rendering-test.gts @@ -608,9 +608,9 @@ module("Plugins | columnReordering", function (hooks) { assert.strictEqual(getColumnOrder(), "B C A D", "pre-test setup"); let order = new ColumnOrder({ - allColumns: () => + columns: () => [{ key: "D" }, { key: "C" }, { key: "B" }, { key: "A" }] as Column[], - availableColumns: () => ({ + visibleColumns: () => ({ A: true, B: true, C: true, From 746dc57c7415b83cb0399b69a54163ab0c13b3f4 Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 15:43:26 +0200 Subject: [PATCH 27/29] lint --- .../tests/plugins/column-reordering/ColumnOrder-test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts index 59a693ba..0b3ab23a 100644 --- a/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts +++ b/test-app/tests/plugins/column-reordering/ColumnOrder-test.ts @@ -364,11 +364,7 @@ module('Plugin | column-reordering | ColumnOrder', function () { module('Backwards compatibility', function () { test('without visibleColumns parameter, all columns are treated as visible', function (assert) { - const COLUMNS = [ - { key: 'A' }, - { key: 'B' }, - { key: 'C' }, - ] as Column[]; + const COLUMNS = [{ key: 'A' }, { key: 'B' }, { key: 'C' }] as Column[]; // Old usage - no visibleColumns parameter const order = new ColumnOrder({ From 9c00a1f4c2e5969bea5a73f3c0a393c53e834438 Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 15:44:20 +0200 Subject: [PATCH 28/29] lint --- table/src/plugins/column-reordering/plugin.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/table/src/plugins/column-reordering/plugin.ts b/table/src/plugins/column-reordering/plugin.ts index c3103442..7a72fcba 100644 --- a/table/src/plugins/column-reordering/plugin.ts +++ b/table/src/plugins/column-reordering/plugin.ts @@ -285,10 +285,13 @@ export class ColumnOrder { } // Default: all columns are visible - return this.args.columns().reduce((acc, col) => { - acc[col.key] = true; - return acc; - }, {} as Record); + return this.args.columns().reduce( + (acc, col) => { + acc[col.key] = true; + return acc; + }, + {} as Record, + ); } /** From c5dc334370cddb2d37794531ef89a82a951d0891 Mon Sep 17 00:00:00 2001 From: johanrd Date: Fri, 3 Oct 2025 15:50:36 +0200 Subject: [PATCH 29/29] lint --- docs-app/public/docs/2-plugins/column-visibility.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs-app/public/docs/2-plugins/column-visibility.md b/docs-app/public/docs/2-plugins/column-visibility.md index b039bf15..b26b654b 100644 --- a/docs-app/public/docs/2-plugins/column-visibility.md +++ b/docs-app/public/docs/2-plugins/column-visibility.md @@ -146,9 +146,7 @@ import { hide, show, } from "@universal-ember/table/plugins/column-visibility"; -import { - ColumnReordering, -} from "@universal-ember/table/plugins/column-reordering"; +import { ColumnReordering } from "@universal-ember/table/plugins/column-reordering"; import { DATA } from "docs-app/sample-data";