From bbe22b41bd1cd6e1295503245f529b71a2998262 Mon Sep 17 00:00:00 2001 From: Jonathan Jackson Date: Fri, 17 May 2019 16:10:58 -0400 Subject: [PATCH 1/7] Ensure that table column widths are recomputed when columns change This fixes an issue where removing a column can leave a blank space in the table because it doesn't recompute column widths. The table's `ResizeSensor` is primarily responsible for noticing resizes and updating widths, but when a column is removed, although the inner `table` element width changes, the container `.ember-table` element does not change its width, and thus the `ResizeSensor` never notices, and the column widths are not recomputed. This modifies the `ember-thead` observer to have it call `fillupHandler` when column count changes. It also changes the observer in `ember-thead` to watch `columns.[]` instead of just `columns`, because in the latter case, the `fillupHandler` will not be called if a column was removed via mutation (e.g. `popObject`). Adds tests for removal via both ways (mutation and `this.set('columns', newColumns)`). Also adds tests that adding columns also causes column widths to be computed. Co-authored-by: Jonathan Jackson --- addon/components/ember-thead/component.js | 13 +- .../components/headers/ember-thead-test.js | 168 ++++++++++++++++++ 2 files changed, 176 insertions(+), 5 deletions(-) create mode 100644 tests/integration/components/headers/ember-thead-test.js diff --git a/addon/components/ember-thead/component.js b/addon/components/ember-thead/component.js index 5f1541d20..598a711fa 100644 --- a/addon/components/ember-thead/component.js +++ b/addon/components/ember-thead/component.js @@ -8,6 +8,7 @@ import { notEmpty, or } from '@ember/object/computed'; import { closest } from '../../-private/utils/element'; import { sortMultiple, compareValues } from '../../-private/utils/sort'; +import { scheduleOnce } from '@ember/runloop'; import ColumnTree, { RESIZE_MODE, FILL_MODE, WIDTH_CONSTRAINT } from '../../-private/column-tree'; @@ -195,7 +196,7 @@ export default Component.extend({ this.addObserver('reorderFunction', this._updateApi); this.addObserver('sorts', this._updateColumnTree); - this.addObserver('columns', this._updateColumnTree); + this.addObserver('columns.[]', this._onColumnsChange); this.addObserver('fillMode', this._updateColumnTree); this.addObserver('resizeMode', this._updateColumnTree); this.addObserver('widthConstraint', this._updateColumnTree); @@ -225,6 +226,11 @@ export default Component.extend({ this.columnTree.set('enableReorder', this.get('enableReorder')); }, + _onColumnsChange() { + this._updateColumnTree(); + scheduleOnce('actions', this, this.fillupHandler); + }, + didInsertElement() { this._super(...arguments); @@ -232,10 +238,7 @@ export default Component.extend({ this.columnTree.registerContainer(this._container); - this._tableResizeSensor = new ResizeSensor( - this._container, - bind(this.fillupHandler.bind(this)) - ); + this._tableResizeSensor = new ResizeSensor(this._container, bind(this, this.fillupHandler)); }, willDestroyElement() { diff --git a/tests/integration/components/headers/ember-thead-test.js b/tests/integration/components/headers/ember-thead-test.js new file mode 100644 index 000000000..9ee6d72eb --- /dev/null +++ b/tests/integration/components/headers/ember-thead-test.js @@ -0,0 +1,168 @@ +import { module, test } from 'qunit'; +import { setupRenderingTest } from 'ember-qunit'; +import { click, render } from '@ember/test-helpers'; +import hbs from 'htmlbars-inline-precompile'; +import TablePage from 'ember-table/test-support/pages/ember-table'; +import { A } from '@ember/array'; + +function tableData() { + return { + rows: [ + { A: 'A', B: 'B', C: 'C', D: 'D', E: 'E' }, + { A: 'A', B: 'B', C: 'C', D: 'D', E: 'E' }, + { A: 'A', B: 'B', C: 'C', D: 'D', E: 'E' }, + ], + columns: [ + { name: 'A', valuePath: 'A', width: 180 }, + { name: 'B', valuePath: 'B', width: 180 }, + { name: 'C', valuePath: 'C', width: 180 }, + { name: 'D', valuePath: 'D', width: 180 }, + ], + newColumn: { name: 'E', valuePath: 'E', width: 180 }, + }; +} + +function sumHeaderWidths(table) { + return table.headers.map(h => h.width).reduce((sum, w) => sum + w, 0); +} + +async function renderTable() { + await render(hbs` + + + {{#ember-table data-test-ember-table=true as |t|}} + {{#t.head + widthConstraint='eq-container' + columns=columns as |h|}} + {{#h.row as |r|}} + {{r.cell}} + {{/h.row}} + {{/t.head}} + + {{#t.body rows=rows as |b|}} + {{#b.row as |r|}} + {{#r.cell as |cellValue|}} + {{cellValue}} + {{/r.cell}} + {{/b.row}} + {{/t.body}} + {{/ember-table}} + `); +} + +async function testColumnRemovals(assert, table) { + let originalWidth = table.width; + let originalContainerWidth = table.containerWidth; + + let currentColumnCount = table.headers.length; + assert.equal(currentColumnCount, 4, 'precond - 4 columns'); + assert.equal(sumHeaderWidths(table), originalWidth, 'precond - headers sum to table width'); + assert.equal( + originalWidth, + originalContainerWidth, + 'precond - table is as wide as its container' + ); + + while (currentColumnCount > 1) { + await click('button#remove-column'); + assert.equal( + table.headers.length, + currentColumnCount - 1, + `column count changes from ${currentColumnCount} -> ${currentColumnCount - 1}` + ); + currentColumnCount -= 1; + + assert.equal( + table.width, + originalWidth, + `table width is same after removal of column #${currentColumnCount}.` + ); + assert.equal( + table.containerWidth, + originalContainerWidth, + `new table container is same size as original container after removal of column #${currentColumnCount}.` + ); + assert.equal( + sumHeaderWidths(table), + originalWidth, + `headers sum to table width after removal of column #${currentColumnCount}` + ); + assert.equal( + table.width, + table.containerWidth, + `new table width is as wide as its container after removal of column #${currentColumnCount}.` + ); + } +} + +async function testColumnAddition(assert, table) { + let originalWidth = table.width; + let originalContainerWidth = table.containerWidth; + + let currentColumnCount = table.headers.length; + assert.equal(currentColumnCount, 4, 'precond - 4 columns'); + assert.equal(sumHeaderWidths(table), originalWidth, 'precond - headers sum to table width'); + assert.equal( + originalWidth, + originalContainerWidth, + 'precond - table is as wide as its container' + ); + + await click('#add-column'); + assert.equal(table.headers.length, 5, 'column is added'); + assert.equal(sumHeaderWidths(table), originalWidth, 'headers sum to table width after adding'); + assert.equal(table.width, originalWidth, 'table width is unchanged'); + assert.equal(table.containerWidth, originalContainerWidth, 'table container width is unchanged'); +} + +module('ember-thead', function(hooks) { + setupRenderingTest(hooks); + + test('table resizes when columns are removed', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', data.columns); + this.set('removeColumn', function() { + this.set('columns', this.get('columns').slice(0, -1)); + }); + + await renderTable(); + await testColumnRemovals(assert, new TablePage()); + }); + + test('table resizes when columns are removed via mutation', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', A(data.columns)); + this.set('removeColumn', function() { + this.get('columns').popObject(); + }); + + await renderTable(); + await testColumnRemovals(assert, new TablePage()); + }); + + test('table resizes when columns are added', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', data.columns); + this.set('addColumn', function() { + this.set('columns', [...data.columns, data.newColumn]); + }); + + await renderTable(); + await testColumnAddition(assert, new TablePage()); + }); + + test('table resizes when columns are added via mutation', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', A(data.columns)); + this.set('addColumn', function() { + this.get('columns').pushObject(data.newColumn); + }); + + await renderTable(); + await testColumnAddition(assert, new TablePage()); + }); +}); From a67a2b1c4fa2fc56b0cf0ef3b749a022c361b62e Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Fri, 17 May 2019 18:29:32 -0400 Subject: [PATCH 2/7] Skip _onColumnsChange when there are no remaining columns --- addon/components/ember-thead/component.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/addon/components/ember-thead/component.js b/addon/components/ember-thead/component.js index 598a711fa..006b86bba 100644 --- a/addon/components/ember-thead/component.js +++ b/addon/components/ember-thead/component.js @@ -227,6 +227,9 @@ export default Component.extend({ }, _onColumnsChange() { + if (this.get('columns.length') === 0) { + return; + } this._updateColumnTree(); scheduleOnce('actions', this, this.fillupHandler); }, From 4c9143f12ebdc9f31b54ff60da5c8c1348a88324 Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Mon, 20 May 2019 11:07:03 -0400 Subject: [PATCH 3/7] Convert ember-thead test to a form that works with older Ember This should make it so that the tests pass on Ember 1.12 and 1.13. Use a special `requestAnimationFrame` waiter that waits 2 RAF turns to ensure that the table's columns are done being laid out when the test starts measuring their dimensions. --- .../components/headers/ember-thead-test.js | 94 ++++++++++--------- 1 file changed, 50 insertions(+), 44 deletions(-) diff --git a/tests/integration/components/headers/ember-thead-test.js b/tests/integration/components/headers/ember-thead-test.js index 9ee6d72eb..1515a5a8e 100644 --- a/tests/integration/components/headers/ember-thead-test.js +++ b/tests/integration/components/headers/ember-thead-test.js @@ -1,9 +1,15 @@ -import { module, test } from 'qunit'; -import { setupRenderingTest } from 'ember-qunit'; -import { click, render } from '@ember/test-helpers'; +import { moduleForComponent, test } from 'ember-qunit'; import hbs from 'htmlbars-inline-precompile'; +import { click } from 'ember-native-dom-helpers'; import TablePage from 'ember-table/test-support/pages/ember-table'; import { A } from '@ember/array'; +import RSVP from 'rsvp'; + +async function rafFinished() { + return new RSVP.Promise(resolve => { + requestAnimationFrame(() => requestAnimationFrame(resolve)); + }); +} function tableData() { return { @@ -26,8 +32,8 @@ function sumHeaderWidths(table) { return table.headers.map(h => h.width).reduce((sum, w) => sum + w, 0); } -async function renderTable() { - await render(hbs` +async function renderTable(context) { + await context.render(hbs` {{#ember-table data-test-ember-table=true as |t|}} @@ -48,6 +54,8 @@ async function renderTable() { {{/t.body}} {{/ember-table}} `); + + await rafFinished(); } async function testColumnRemovals(assert, table) { @@ -115,54 +123,52 @@ async function testColumnAddition(assert, table) { assert.equal(table.containerWidth, originalContainerWidth, 'table container width is unchanged'); } -module('ember-thead', function(hooks) { - setupRenderingTest(hooks); - - test('table resizes when columns are removed', async function(assert) { - let data = tableData(); - this.set('rows', data.rows); - this.set('columns', data.columns); - this.set('removeColumn', function() { - this.set('columns', this.get('columns').slice(0, -1)); - }); +moduleForComponent('ember-thead', '[Unit] ember-thead', { integration: true }); - await renderTable(); - await testColumnRemovals(assert, new TablePage()); +test('table resizes when columns are removed', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', data.columns); + this.on('removeColumn', function() { + this.set('columns', this.get('columns').slice(0, -1)); }); - test('table resizes when columns are removed via mutation', async function(assert) { - let data = tableData(); - this.set('rows', data.rows); - this.set('columns', A(data.columns)); - this.set('removeColumn', function() { - this.get('columns').popObject(); - }); + await renderTable(this); + await testColumnRemovals(assert, new TablePage(), this); +}); - await renderTable(); - await testColumnRemovals(assert, new TablePage()); +test('table resizes when columns are removed via mutation', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', A(data.columns)); + this.on('removeColumn', function() { + this.get('columns').popObject(); }); - test('table resizes when columns are added', async function(assert) { - let data = tableData(); - this.set('rows', data.rows); - this.set('columns', data.columns); - this.set('addColumn', function() { - this.set('columns', [...data.columns, data.newColumn]); - }); + await renderTable(this); + await testColumnRemovals(assert, new TablePage(), this); +}); - await renderTable(); - await testColumnAddition(assert, new TablePage()); +test('table resizes when columns are added', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', data.columns); + this.on('addColumn', function() { + this.set('columns', [...data.columns, data.newColumn]); }); - test('table resizes when columns are added via mutation', async function(assert) { - let data = tableData(); - this.set('rows', data.rows); - this.set('columns', A(data.columns)); - this.set('addColumn', function() { - this.get('columns').pushObject(data.newColumn); - }); + await renderTable(this); + await testColumnAddition(assert, new TablePage(), this); +}); - await renderTable(); - await testColumnAddition(assert, new TablePage()); +test('table resizes when columns are added via mutation', async function(assert) { + let data = tableData(); + this.set('rows', data.rows); + this.set('columns', A(data.columns)); + this.on('addColumn', function() { + this.get('columns').pushObject(data.newColumn); }); + + await renderTable(this); + await testColumnAddition(assert, new TablePage(), this); }); From 0fc27a4297fbd3fb83750dae47af347a5ac69d56 Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Mon, 20 May 2019 11:11:00 -0400 Subject: [PATCH 4/7] Fix arguments for test helper functions --- tests/integration/components/headers/ember-thead-test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integration/components/headers/ember-thead-test.js b/tests/integration/components/headers/ember-thead-test.js index 1515a5a8e..0a094cfb5 100644 --- a/tests/integration/components/headers/ember-thead-test.js +++ b/tests/integration/components/headers/ember-thead-test.js @@ -134,7 +134,7 @@ test('table resizes when columns are removed', async function(assert) { }); await renderTable(this); - await testColumnRemovals(assert, new TablePage(), this); + await testColumnRemovals(assert, new TablePage()); }); test('table resizes when columns are removed via mutation', async function(assert) { @@ -146,7 +146,7 @@ test('table resizes when columns are removed via mutation', async function(asser }); await renderTable(this); - await testColumnRemovals(assert, new TablePage(), this); + await testColumnRemovals(assert, new TablePage()); }); test('table resizes when columns are added', async function(assert) { @@ -158,7 +158,7 @@ test('table resizes when columns are added', async function(assert) { }); await renderTable(this); - await testColumnAddition(assert, new TablePage(), this); + await testColumnAddition(assert, new TablePage()); }); test('table resizes when columns are added via mutation', async function(assert) { @@ -170,5 +170,5 @@ test('table resizes when columns are added via mutation', async function(assert) }); await renderTable(this); - await testColumnAddition(assert, new TablePage(), this); + await testColumnAddition(assert, new TablePage()); }); From 280efd56c25b87985b0b30d0932ba898255b5df3 Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Mon, 20 May 2019 11:29:25 -0400 Subject: [PATCH 5/7] Use back-compatible template for ember 1.12 and 1.13 --- .../components/headers/ember-thead-test.js | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/tests/integration/components/headers/ember-thead-test.js b/tests/integration/components/headers/ember-thead-test.js index 0a094cfb5..4593bebb9 100644 --- a/tests/integration/components/headers/ember-thead-test.js +++ b/tests/integration/components/headers/ember-thead-test.js @@ -37,21 +37,22 @@ async function renderTable(context) { {{#ember-table data-test-ember-table=true as |t|}} - {{#t.head + {{#ember-thead + api=t widthConstraint='eq-container' columns=columns as |h|}} - {{#h.row as |r|}} - {{r.cell}} - {{/h.row}} - {{/t.head}} - - {{#t.body rows=rows as |b|}} - {{#b.row as |r|}} - {{#r.cell as |cellValue|}} + {{#ember-tr api=h as |r|}} + {{ember-th api=r}} + {{/ember-tr}} + {{/ember-thead}} + + {{#ember-tbody api=t rows=rows as |b|}} + {{#ember-tr api=b as |r|}} + {{#ember-td api=r as |cellValue|}} {{cellValue}} - {{/r.cell}} - {{/b.row}} - {{/t.body}} + {{/ember-td}} + {{/ember-tr}} + {{/ember-tbody}} {{/ember-table}} `); From c8814b31ded4b082ab59cbe7dc0fd3f594fe540d Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Mon, 20 May 2019 12:08:11 -0400 Subject: [PATCH 6/7] Add Ember-1.12 compatibility for `this.set` and `this.get` in test --- .../components/headers/ember-thead-test.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/integration/components/headers/ember-thead-test.js b/tests/integration/components/headers/ember-thead-test.js index 4593bebb9..bc05fdebf 100644 --- a/tests/integration/components/headers/ember-thead-test.js +++ b/tests/integration/components/headers/ember-thead-test.js @@ -124,6 +124,17 @@ async function testColumnAddition(assert, table) { assert.equal(table.containerWidth, originalContainerWidth, 'table container width is unchanged'); } +// In Ember-1.12, the `this` context has a `context` property itself that is used to `get` or `set`. +// This function provides Ember-1.12 compatibility for `this.get` +function compatGet(context, propName) { + return context.get ? context.get(propName) : context.context.get(propName); +} + +// This function provides Ember-1.12 compatibility for `this.set` +function compatSet(context, propName, value) { + return context.set ? context.set(propName, value) : context.context.set(propName, value); +} + moduleForComponent('ember-thead', '[Unit] ember-thead', { integration: true }); test('table resizes when columns are removed', async function(assert) { @@ -131,7 +142,7 @@ test('table resizes when columns are removed', async function(assert) { this.set('rows', data.rows); this.set('columns', data.columns); this.on('removeColumn', function() { - this.set('columns', this.get('columns').slice(0, -1)); + compatSet(this, 'columns', compatGet(this, 'columns').slice(0, -1)); }); await renderTable(this); @@ -143,7 +154,7 @@ test('table resizes when columns are removed via mutation', async function(asser this.set('rows', data.rows); this.set('columns', A(data.columns)); this.on('removeColumn', function() { - this.get('columns').popObject(); + compatGet(this, 'columns').popObject(); }); await renderTable(this); @@ -155,7 +166,7 @@ test('table resizes when columns are added', async function(assert) { this.set('rows', data.rows); this.set('columns', data.columns); this.on('addColumn', function() { - this.set('columns', [...data.columns, data.newColumn]); + compatSet(this, 'columns', [...data.columns, data.newColumn]); }); await renderTable(this); @@ -167,7 +178,7 @@ test('table resizes when columns are added via mutation', async function(assert) this.set('rows', data.rows); this.set('columns', A(data.columns)); this.on('addColumn', function() { - this.get('columns').pushObject(data.newColumn); + compatGet(this, 'columns').pushObject(data.newColumn); }); await renderTable(this); From 8575206b26b028ec1ec724e34fa99ea067ac6772 Mon Sep 17 00:00:00 2001 From: Cory Forsyth Date: Mon, 20 May 2019 13:40:25 -0400 Subject: [PATCH 7/7] Add comment about the `rafFinished` helper --- tests/integration/components/headers/ember-thead-test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/integration/components/headers/ember-thead-test.js b/tests/integration/components/headers/ember-thead-test.js index bc05fdebf..29bb79c84 100644 --- a/tests/integration/components/headers/ember-thead-test.js +++ b/tests/integration/components/headers/ember-thead-test.js @@ -5,6 +5,11 @@ import TablePage from 'ember-table/test-support/pages/ember-table'; import { A } from '@ember/array'; import RSVP from 'rsvp'; +// This is a "waiter"-style helper to use to ensure that +// the interior of the ember-table has finished all of its +// layout-related logic has finished. If we don't use this to +// wait after `render`, the column widths will not have been +// set yet and the tests in this module are moot. async function rafFinished() { return new RSVP.Promise(resolve => { requestAnimationFrame(() => requestAnimationFrame(resolve));