From d5669a1d9c07680540d084dad6e1ef06faca0357 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Wed, 26 May 2021 10:58:27 -0400 Subject: [PATCH 1/2] fix(demo): we should be able to move row(s) and keep selections - grid state should only be allowed to change row selection only when Pagination is enabled - refactor some part of the code to use more optional chaining (?.) syntax for cleaner and smaller code - move around some of the Cypress tests so that it makes sure that moving rows will keep selections --- .../src/examples/example07.ts | 62 ++++++++++++------- .../common/src/services/gridState.service.ts | 34 +++++----- packages/common/src/services/sort.service.ts | 4 +- test/cypress/integration/example07.spec.js | 42 ++++++------- 4 files changed, 81 insertions(+), 61 deletions(-) diff --git a/examples/webpack-demo-vanilla-bundle/src/examples/example07.ts b/examples/webpack-demo-vanilla-bundle/src/examples/example07.ts index 4ce91f210..f32f8c070 100644 --- a/examples/webpack-demo-vanilla-bundle/src/examples/example07.ts +++ b/examples/webpack-demo-vanilla-bundle/src/examples/example07.ts @@ -201,7 +201,7 @@ export class Example7 { }, enableRowMoveManager: true, rowMoveManager: { - // when using Row Move + Row Selection, you want to enable the following 2 flags so it doesn't cancel row selection + // when using Row Move + Row Selection, you want to move only a single row and we will enable the following flags so it doesn't cancel row selection singleRowMove: true, disableRowSelection: true, cancelEditOnDrag: true, @@ -297,7 +297,7 @@ export class Example7 { return collection.sort((item1, item2) => item1.value - item2.value); } - onBeforeMoveRow(e, data) { + onBeforeMoveRow(e: Event, data: any) { for (let i = 0; i < data.rows.length; i++) { // no point in moving before or after itself if (data.rows[i] === data.insertBefore || data.rows[i] === data.insertBefore - 1) { @@ -308,32 +308,52 @@ export class Example7 { return true; } - onMoveRows(_e, args) { - const extractedRows = []; - const rows = args.rows; + onMoveRows(_e: Event, args: any) { + // rows and insertBefore references, + // note that these references are assuming that the dataset isn't filtered at all + // which is not always the case so we will recalcualte them and we won't use these reference afterward + const rows = args.rows as number[]; const insertBefore = args.insertBefore; - const left = this.dataset.slice(0, insertBefore); - const right = this.dataset.slice(insertBefore, this.dataset.length); - rows.sort((a, b) => a - b); - for (let i = 0; i < rows.length; i++) { - extractedRows.push(this.dataset[rows[i]]); + const extractedRows = []; + + // when moving rows, we need to cancel any sorting that might happen + // we can do this by providing an undefined sort comparer + // which basically destroys the current sort comparer without resorting the dataset, it basically keeps the previous sorting + this.sgb.dataView.sort(undefined, true); + + // the dataset might be filtered/sorted, + // so we need to get the same dataset as the one that the SlickGrid DataView uses + const tmpDataset = this.sgb.dataView.getItems(); + const filteredItems = this.sgb.dataView.getFilteredItems(); + + const itemOnRight = this.sgb.dataView.getItem(insertBefore); + const insertBeforeFilteredIdx = this.sgb.dataView.getIdxById(itemOnRight.id); + + const filteredRowItems = []; + rows.forEach(row => filteredRowItems.push(filteredItems[row])); + const filteredRows = filteredRowItems.map(item => this.sgb.dataView.getIdxById(item.id)); + + const left = tmpDataset.slice(0, insertBeforeFilteredIdx); + const right = tmpDataset.slice(insertBeforeFilteredIdx, tmpDataset.length); + + // convert into a final new dataset that has the new order + // we need to resort with + rows.sort((a: number, b: number) => a - b); + for (const filteredRow of filteredRows) { + extractedRows.push(tmpDataset[filteredRow]); } - rows.reverse(); - for (let i = 0; i < rows.length; i++) { - const row = rows[i]; - if (row < insertBefore) { + filteredRows.reverse(); + for (const row of filteredRows) { + if (row < insertBeforeFilteredIdx) { left.splice(row, 1); } else { - right.splice(row - insertBefore, 1); + right.splice(row - insertBeforeFilteredIdx, 1); } } - this.dataset = left.concat(extractedRows.concat(right)); - const selectedRows = []; - for (let i = 0; i < rows.length; i++) { - selectedRows.push(left.length + i); - } - args.grid.resetActiveCell(); + // final updated dataset, we need to overwrite the DataView dataset (and our local one) with this new dataset that has a new order + const finalDataset = left.concat(extractedRows.concat(right)); + this.dataset = finalDataset; this.sgb.dataset = this.dataset; // update dataset and re-render the grid } diff --git a/packages/common/src/services/gridState.service.ts b/packages/common/src/services/gridState.service.ts index a09741ad2..a43fe5cf9 100644 --- a/packages/common/src/services/gridState.service.ts +++ b/packages/common/src/services/gridState.service.ts @@ -155,7 +155,7 @@ export class GridStateService { } if (this.hasRowSelectionEnabled()) { - const currentRowSelection = this.getCurrentRowSelections(args && args.requestRefreshRowFilteredRow); + const currentRowSelection = this.getCurrentRowSelections(args?.requestRefreshRowFilteredRow); if (currentRowSelection) { gridState.rowSelection = currentRowSelection; } @@ -181,7 +181,7 @@ export class GridStateService { if (gridColumns && Array.isArray(gridColumns)) { gridColumns.forEach((column: Column) => { - if (column && column.id) { + if (column?.id) { currentColumns.push({ columnId: column.id as string, cssClass: column.cssClass || '', @@ -207,7 +207,7 @@ export class GridStateService { if (currentColumns && Array.isArray(currentColumns)) { currentColumns.forEach((currentColumn: CurrentColumn) => { const gridColumn: Column | undefined = gridColumns.find((c: Column) => c.id === currentColumn.columnId); - if (gridColumn && gridColumn.id) { + if (gridColumn?.id) { columns.push({ ...gridColumn, cssClass: currentColumn.cssClass, @@ -243,10 +243,10 @@ export class GridStateService { getCurrentFilters(): CurrentFilter[] | null { if (this._gridOptions && this._gridOptions.backendServiceApi) { const backendService = this._gridOptions.backendServiceApi.service; - if (backendService && backendService.getCurrentFilters) { + if (backendService?.getCurrentFilters) { return backendService.getCurrentFilters() as CurrentFilter[]; } - } else if (this.filterService && this.filterService.getCurrentLocalFilters) { + } else if (this.filterService?.getCurrentLocalFilters) { return this.filterService.getCurrentLocalFilters(); } return null; @@ -258,9 +258,9 @@ export class GridStateService { */ getCurrentPagination(): CurrentPagination | null { if (this._gridOptions.enablePagination) { - if (this._gridOptions && this._gridOptions.backendServiceApi) { + if (this._gridOptions?.backendServiceApi) { const backendService = this._gridOptions.backendServiceApi.service; - if (backendService && backendService.getCurrentPagination) { + if (backendService?.getCurrentPagination) { return backendService.getCurrentPagination(); } } else { @@ -298,12 +298,12 @@ export class GridStateService { * @return current sorters */ getCurrentSorters(): CurrentSorter[] | null { - if (this._gridOptions && this._gridOptions.backendServiceApi) { + if (this._gridOptions?.backendServiceApi) { const backendService = this._gridOptions.backendServiceApi.service; - if (backendService && backendService.getCurrentSorters) { + if (backendService?.getCurrentSorters) { return backendService.getCurrentSorters() as CurrentSorter[]; } - } else if (this.sortService && this.sortService.getCurrentLocalSorters) { + } else if (this.sortService?.getCurrentLocalSorters) { return this.sortService.getCurrentLocalSorters(); } return null; @@ -312,7 +312,7 @@ export class GridStateService { /** Check whether the row selection needs to be preserved */ needToPreserveRowSelection(): boolean { let preservedRowSelection = false; - if (this._gridOptions && this._gridOptions.dataView && this._gridOptions.dataView.hasOwnProperty('syncGridSelection')) { + if (this._gridOptions?.dataView && this._gridOptions.dataView.hasOwnProperty('syncGridSelection')) { const syncGridSelection = this._gridOptions.dataView.syncGridSelection; if (typeof syncGridSelection === 'boolean') { preservedRowSelection = this._gridOptions.dataView.syncGridSelection as boolean; @@ -353,8 +353,8 @@ export class GridStateService { resetRowSelectionWhenRequired() { if (!this.needToPreserveRowSelection() && (this._gridOptions.enableRowSelection || this._gridOptions.enableCheckboxSelector)) { // this also requires the Row Selection Model to be registered as well - const rowSelectionExtension = this.extensionService && this.extensionService.getExtensionByName && this.extensionService.getExtensionByName(ExtensionName.rowSelection); - if (rowSelectionExtension && rowSelectionExtension.instance) { + const rowSelectionExtension = this.extensionService?.getExtensionByName?.(ExtensionName.rowSelection); + if (rowSelectionExtension?.instance) { this._grid.setSelectedRows([]); } } @@ -433,12 +433,12 @@ export class GridStateService { * @param event name */ private bindExtensionAddonEventToGridStateChange(extensionName: ExtensionName, eventName: string) { - const extension = this.extensionService && this.extensionService.getExtensionByName && this.extensionService.getExtensionByName(extensionName); - const slickEvent = extension && extension.instance && extension.instance[eventName]; + const extension = this.extensionService?.getExtensionByName?.(extensionName); + const slickEvent = extension?.instance?.[eventName]; if (slickEvent && typeof slickEvent.subscribe === 'function') { (this._eventHandler as SlickEventHandler>).subscribe(slickEvent, (_e, args) => { - const columns: Column[] = args && args.columns; + const columns: Column[] = args?.columns; const currentColumns: CurrentColumn[] = this.getAssociatedCurrentColumns(columns); this.pubSubService.publish('onGridStateChanged', { change: { newValues: currentColumns, type: GridStateType.columns }, gridState: this.getCurrentGridState() }); }); @@ -547,7 +547,7 @@ export class GridStateService { // this could happen if the previous step was a page change const shouldBeSelectedRowIndexes = this._dataView.mapIdsToRows(this._selectedRowDataContextIds || []); const currentSelectedRowIndexes = this._grid.getSelectedRows(); - if (!dequal(shouldBeSelectedRowIndexes, currentSelectedRowIndexes)) { + if (!dequal(shouldBeSelectedRowIndexes, currentSelectedRowIndexes) && this._gridOptions.enablePagination) { this._grid.setSelectedRows(shouldBeSelectedRowIndexes); } diff --git a/packages/common/src/services/sort.service.ts b/packages/common/src/services/sort.service.ts index 398cb70ad..fb7c9b9dd 100644 --- a/packages/common/src/services/sort.service.ts +++ b/packages/common/src/services/sort.service.ts @@ -177,8 +177,8 @@ export class SortService { } } } else if (this._isBackendGrid) { - const backendService = this._gridOptions && this._gridOptions.backendServiceApi && this._gridOptions.backendServiceApi.service; - if (backendService && backendService.clearSorters) { + const backendService = this._gridOptions?.backendServiceApi?.service; + if (backendService?.clearSorters) { backendService.clearSorters(); } } diff --git a/test/cypress/integration/example07.spec.js b/test/cypress/integration/example07.spec.js index cfa9a2440..7385e4b66 100644 --- a/test/cypress/integration/example07.spec.js +++ b/test/cypress/integration/example07.spec.js @@ -71,24 +71,7 @@ describe('Example 07 - Row Move & Checkbox Selector Selector Plugins', { retries cy.get(`[style="top:${GRID_ROW_HEIGHT * 7}px"] > .slick-cell:nth(1) input[type="checkbox"]:checked`).should('have.length', 1); }); - it('should uncheck all rows', () => { - // click twice to check then uncheck all - cy.get('.slick-column-name > input[type=checkbox]') - .click({ force: true }); - cy.get('.slick-column-name > input[type=checkbox]') - .click({ force: true }); - }); - - it('should have 0 row selected count shown in the grid left footer', () => { - cy.get('.slick-custom-footer') - .find('div.left-footer') - .should($span => { - const text = removeExtraSpaces($span.text()); // remove all white spaces - expect(text).to.eq(`0 items selected`); - }); - }); - - it('should drag opened Row Detail to another position in the grid', () => { + it('should drag row to another position in the grid', () => { cy.get('[style="top:45px"] > .slick-cell.cell-reorder').as('moveIconTask1'); cy.get('[style="top:135px"] > .slick-cell.cell-reorder').as('moveIconTask3'); @@ -103,7 +86,7 @@ describe('Example 07 - Row Move & Checkbox Selector Selector Plugins', { retries .trigger('mouseup', 'bottomRight', { force: true }); cy.get('input[type="checkbox"]:checked') - .should('have.length', 0); + .should('have.length', 4); }); it('should expect row to be moved to another row index', () => { @@ -117,10 +100,27 @@ describe('Example 07 - Row Move & Checkbox Selector Selector Plugins', { retries cy.get(`[style="top:${GRID_ROW_HEIGHT * 4}px"] > .slick-cell:nth(2)`).should('contain', 'Task 4'); cy.get('input[type="checkbox"]:checked') - .should('have.length', 0); + .should('have.length', 4); + }); + + it('should uncheck all rows', () => { + // click twice to check then uncheck all + cy.get('.slick-column-name > input[type=checkbox]') + .click({ force: true }); + cy.get('.slick-column-name > input[type=checkbox]') + .click({ force: true }); + }); + + it('should have 0 row selected count shown in the grid left footer', () => { + cy.get('.slick-custom-footer') + .find('div.left-footer') + .should($span => { + const text = removeExtraSpaces($span.text()); // remove all white spaces + expect(text).to.eq(`0 items selected`); + }); }); - it('should select 2 rows (Task 3,4), then move row and expect the 2 rows to still be selected without any others', () => { + it('should select 2 rows (Task 3,4), then move the rows and expect both rows to still be selected without any other rows', () => { cy.get(`[style="top:${GRID_ROW_HEIGHT * 2}px"] > .slick-cell:nth(1)`).click(); cy.get(`[style="top:${GRID_ROW_HEIGHT * 4}px"] > .slick-cell:nth(1)`).click(); From 5b8eb70221cf60493581ef455f8a6d0d12f6bbf4 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Wed, 26 May 2021 11:46:22 -0400 Subject: [PATCH 2/2] refactor: address review comment --- packages/common/src/services/gridState.service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/common/src/services/gridState.service.ts b/packages/common/src/services/gridState.service.ts index a43fe5cf9..48bf95837 100644 --- a/packages/common/src/services/gridState.service.ts +++ b/packages/common/src/services/gridState.service.ts @@ -257,8 +257,8 @@ export class GridStateService { * @return current pagination state */ getCurrentPagination(): CurrentPagination | null { - if (this._gridOptions.enablePagination) { - if (this._gridOptions?.backendServiceApi) { + if (this._gridOptions?.enablePagination) { + if (this._gridOptions.backendServiceApi) { const backendService = this._gridOptions.backendServiceApi.service; if (backendService?.getCurrentPagination) { return backendService.getCurrentPagination();