diff --git a/src/vaadin-grid-dynamic-columns-mixin.js b/src/vaadin-grid-dynamic-columns-mixin.js index 114c62837..47337c6c3 100644 --- a/src/vaadin-grid-dynamic-columns-mixin.js +++ b/src/vaadin-grid-dynamic-columns-mixin.js @@ -87,11 +87,14 @@ export const DynamicColumnsMixin = (superClass) => if (rowDetailsTemplate && this._rowDetailsTemplate !== rowDetailsTemplate) { this._rowDetailsTemplate = rowDetailsTemplate; } - - if ( - info.addedNodes.filter(this._isColumnElement).length > 0 || - info.removedNodes.filter(this._isColumnElement).length > 0 - ) { + const hasColumnElements = (nodeCollection) => nodeCollection.filter(this._isColumnElement).length > 0; + if (hasColumnElements(info.addedNodes) || hasColumnElements(info.removedNodes)) { + const allRemovedCells = info.removedNodes.flatMap((c) => c._allCells); + const filterNotConnected = (element) => + allRemovedCells.filter((cell) => cell._content.contains(element)).length; + + this.__removeSorters(this._sorters.filter(filterNotConnected)); + this.__removeFilters(this._filters.filter(filterNotConnected)); this._updateColumnTree(); } diff --git a/src/vaadin-grid-filter-mixin.js b/src/vaadin-grid-filter-mixin.js index 64dc5c609..6a1156598 100644 --- a/src/vaadin-grid-filter-mixin.js +++ b/src/vaadin-grid-filter-mixin.js @@ -29,13 +29,34 @@ export const FilterMixin = (superClass) => /** @private */ _filterChanged(e) { - if (this._filters.indexOf(e.target) === -1) { - this._filters.push(e.target); + e.stopPropagation(); + + this.__addFilter(e.target); + this.__applyFilters(); + } + + /** @private */ + __removeFilters(filtersToRemove) { + if (filtersToRemove.length == 0) { + return; } - e.stopPropagation(); + this._filters = this._filters.filter((filter) => filtersToRemove.indexOf(filter) < 0); + this.__applyFilters(); + } - if (this.dataProvider) { + /** @private */ + __addFilter(filter) { + const filterIndex = this._filters.indexOf(filter); + + if (filterIndex === -1) { + this._filters.push(filter); + } + } + + /** @private */ + __applyFilters() { + if (this.dataProvider && this.isAttached) { this.clearCache(); } } diff --git a/src/vaadin-grid-sort-mixin.js b/src/vaadin-grid-sort-mixin.js index 1d67ea6f7..0d9d3ed41 100644 --- a/src/vaadin-grid-sort-mixin.js +++ b/src/vaadin-grid-sort-mixin.js @@ -51,31 +51,61 @@ export const SortMixin = (superClass) => /** @private */ _onSorterChanged(e) { const sorter = e.target; + e.stopPropagation(); + this.__updateSorter(sorter); + this.__applySorters(); + } + + /** @private */ + __removeSorters(sortersToRemove) { + if (sortersToRemove.length == 0) { + return; + } + + this._sorters = this._sorters.filter((sorter) => sortersToRemove.indexOf(sorter) < 0); + if (this.multiSort) { + this.__updateSortOrders(); + } + this.__applySorters(); + } + + /** @private */ + __updateSortOrders() { + this._sorters.forEach((sorter, index) => (sorter._order = this._sorters.length > 1 ? index : null), this); + } + + /** @private */ + __updateSorter(sorter) { + if (!sorter.direction && this._sorters.indexOf(sorter) === -1) { + return; + } - this._removeArrayItem(this._sorters, sorter); sorter._order = null; if (this.multiSort) { + this._removeArrayItem(this._sorters, sorter); if (sorter.direction) { this._sorters.unshift(sorter); } - - this._sorters.forEach((sorter, index) => (sorter._order = this._sorters.length > 1 ? index : null), this); + this.__updateSortOrders(); } else { if (sorter.direction) { - this._sorters.forEach((sorter) => { + const otherSorters = this._sorters.filter((s) => s != sorter); + this._sorters = [sorter]; + otherSorters.forEach((sorter) => { sorter._order = null; sorter.direction = null; }); - this._sorters = [sorter]; } } + } - e.stopPropagation(); - + /** @private */ + __applySorters() { if ( this.dataProvider && - // No need to clear cache if sorters didn't change + // No need to clear cache if sorters didn't change and grid is attached + this.isAttached && JSON.stringify(this._previousSorters) !== JSON.stringify(this._mapSorters()) ) { this.clearCache(); diff --git a/src/vaadin-grid-sorter.js b/src/vaadin-grid-sorter.js index 8d3d1ef3b..12849adcf 100644 --- a/src/vaadin-grid-sorter.js +++ b/src/vaadin-grid-sorter.js @@ -145,13 +145,13 @@ class GridSorterElement extends ThemableMixin(DirMixin(PolymerElement)) { /** @private */ _isConnected: { type: Boolean, - value: false + observer: '__isConnectedChanged' } }; } static get observers() { - return ['_pathOrDirectionChanged(path, direction, _isConnected)']; + return ['_pathOrDirectionChanged(path, direction)']; } /** @protected */ @@ -173,14 +173,26 @@ class GridSorterElement extends ThemableMixin(DirMixin(PolymerElement)) { } /** @private */ - _pathOrDirectionChanged(path, direction, isConnected) { - if (path === undefined || direction === undefined || isConnected === undefined) { + _pathOrDirectionChanged() { + this.__dispatchSorterChangedEvenIfPossible(); + } + + /** @private */ + __isConnectedChanged(newValue, oldValue) { + if (oldValue === false) { return; } - if (isConnected) { - this.dispatchEvent(new CustomEvent('sorter-changed', { bubbles: true, composed: true })); + this.__dispatchSorterChangedEvenIfPossible(); + } + + /** @private */ + __dispatchSorterChangedEvenIfPossible() { + if (this.path === undefined || this.direction === undefined || !this._isConnected) { + return; } + + this.dispatchEvent(new CustomEvent('sorter-changed', { bubbles: true, composed: true })); } /** @private */ diff --git a/test/filtering.test.js b/test/filtering.test.js index d951241a6..2f4cdf1fc 100644 --- a/test/filtering.test.js +++ b/test/filtering.test.js @@ -3,7 +3,14 @@ import sinon from 'sinon'; import { fixtureSync } from '@open-wc/testing-helpers'; import { PolymerElement, html } from '@polymer/polymer/polymer-element.js'; import { flush } from '@polymer/polymer/lib/utils/flush.js'; -import { flushGrid, getBodyCellContent, getHeaderCellContent, listenOnce, scrollToEnd } from './helpers.js'; +import { + flushGrid, + getBodyCellContent, + getHeaderCellContent, + listenOnce, + scrollToEnd, + getVisibleItems +} from './helpers.js'; import '../vaadin-grid.js'; import '../vaadin-grid-filter.js'; import '../vaadin-grid-filter-column.js'; @@ -67,7 +74,7 @@ describe('filter', () => { }); describe('filtering', () => { - let grid; + let grid, filter; beforeEach(() => { grid = fixtureSync(` @@ -95,6 +102,7 @@ describe('filtering', () => { if (grid._observer.flush) { grid._observer.flush(); } + filter = grid._filters[0]; }); it('should have filters', () => { @@ -107,6 +115,21 @@ describe('filtering', () => { grid._filters.forEach((filter) => expect(filter.$.filter.clientHeight).to.be.greaterThan(0)); }); + it('should not keep references to filters when column is removed', () => { + grid.removeChild(grid.firstElementChild); + flushGrid(grid); + expect(grid._filters).to.not.contain(filter); + }); + + it('should keep references to filters for columns that are not removed', () => { + expect(grid._filters.length).to.eql(2); + expect(grid._filters[1].path).to.eql('last'); + grid.removeChild(grid.firstElementChild.nextElementSibling); + flushGrid(grid); + expect(grid._filters.length).to.eql(1); + expect(grid._filters[0].path).to.eql('first'); + }); + it('should pass filters to dataProvider', (done) => { grid.size = 10; @@ -190,7 +213,7 @@ describe('filtering', () => { }); describe('array data provider', () => { - let grid; + let grid, filterFirst, filterSecond; beforeEach(() => { grid = fixtureSync(` @@ -216,8 +239,11 @@ describe('array data provider', () => { flushGrid(grid); flushFilters(grid); - grid._filters[0].value = ''; - grid._filters[1].value = ''; + filterFirst = grid._filters[0]; + filterSecond = grid._filters[1]; + + filterFirst.value = ''; + filterSecond.value = ''; flushFilters(grid); grid.items = [ { @@ -252,6 +278,32 @@ describe('array data provider', () => { expect(getBodyCellContent(grid, 0, 0).innerText).to.equal('bar'); }); + it('should update filtering when column is removed', () => { + filterFirst.value = 'bar'; + flushFilters(grid); + + grid.removeChild(grid.firstElementChild); + flushGrid(grid); + + expect(getVisibleItems(grid).length).to.equal(3); + }); + + it('should not filter items before grid is re-attached', () => { + filterFirst.value = 'bar'; + flushFilters(grid); + + const parentNode = grid.parentNode; + parentNode.removeChild(grid); + grid.removeChild(grid.firstElementChild); + flushGrid(grid); + + expect(Object.keys(grid._cache.items).length).to.equal(1); + + parentNode.appendChild(grid); + + expect(Object.keys(grid._cache.items).length).to.equal(3); + }); + it('should sort filtered items', () => { grid._filters[1].value = 'r'; grid.querySelector('vaadin-grid-sorter').direction = 'asc'; diff --git a/test/helpers.js b/test/helpers.js index ac0832729..0961a80e9 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -113,7 +113,7 @@ export const isVisible = (el) => { ); }; -const getVisibleItems = (grid) => { +export const getVisibleItems = (grid) => { flushGrid(grid); const rows = grid.$.items.children; const visibleRows = []; diff --git a/test/sorting.test.js b/test/sorting.test.js index 0dbd033aa..81be66712 100644 --- a/test/sorting.test.js +++ b/test/sorting.test.js @@ -76,6 +76,89 @@ describe('sorting', () => { }); }); + describe('DOM operations', () => { + let grid, sorterFirst, sorterSecond, sorterThird, columnFirst, columnThird; + + beforeEach(async () => { + grid = fixtureSync(` + + + + + + `); + await nextFrame(); + + // TODO: find better way to select + columnFirst = grid.querySelectorAll('vaadin-grid-sort-column')[0]; + columnThird = grid.querySelectorAll('vaadin-grid-sort-column')[2]; + + sorterFirst = getHeaderCellContent(grid, 0, 0).querySelector('vaadin-grid-sorter'); + sorterSecond = getHeaderCellContent(grid, 0, 1).querySelector('vaadin-grid-sorter'); + sorterThird = getHeaderCellContent(grid, 0, 2).querySelector('vaadin-grid-sorter'); + + grid.items = [ + { first: '1', second: '2', third: '3' }, + { first: '2', second: '3', third: '1' }, + { first: '3', second: '1', third: '2' } + ]; + + flushGrid(grid); + }); + + it('should preserve sort order for sorters when grid is re-attached', () => { + click(sorterSecond); + const parentNode = grid.parentNode; + parentNode.removeChild(grid); + parentNode.appendChild(grid); + + expect(sorterFirst._order).to.equal(2); + expect(sorterSecond._order).to.equal(0); + expect(sorterThird._order).to.equal(1); + }); + + it('should not keep references to sorters when column is removed', () => { + grid.removeChild(columnFirst); + flushGrid(grid); + expect(grid._sorters).to.not.contain(sorterFirst); + }); + + it('should update sorting when column is removed', () => { + grid.removeChild(columnThird); + flushGrid(grid); + + expect(getBodyCellContent(grid, 0, 0).innerText).to.equal('3'); + expect(getBodyCellContent(grid, 1, 0).innerText).to.equal('1'); + expect(getBodyCellContent(grid, 2, 0).innerText).to.equal('2'); + + expect(getBodyCellContent(grid, 0, 1).innerText).to.equal('1'); + expect(getBodyCellContent(grid, 1, 1).innerText).to.equal('2'); + expect(getBodyCellContent(grid, 2, 1).innerText).to.equal('3'); + }); + + it('should update sort order when column removed and grid is not attached', () => { + const parentNode = grid.parentNode; + parentNode.removeChild(grid); + + grid.removeChild(columnThird); + flushGrid(grid); + expect(sorterFirst._order).to.equal(1); + expect(sorterSecond._order).to.equal(0); + }); + + it('should not sort items before grid is re-attached', () => { + const parentNode = grid.parentNode; + parentNode.removeChild(grid); + + grid.removeChild(columnThird); + flushGrid(grid); + expect(getBodyCellContent(grid, 0, 1).innerText).to.equal('2'); + + parentNode.appendChild(grid); + expect(getBodyCellContent(grid, 0, 1).innerText).to.equal('1'); + }); + }); + describe('grid', () => { let grid, sorterFirst, sorterLast; @@ -286,7 +369,7 @@ describe('sorting', () => { grid.dataProvider.resetHistory(); sorterFirst.direction = 'desc'; - expect(grid.dataProvider.args[1][0].sortOrders.length).to.eql(1); + expect(grid.dataProvider.args[0][0].sortOrders.length).to.eql(1); }); it('should remove order from sorters', () => {