From 91a3b096981f026fa318f8a41b2db4eacef552fe Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Tue, 3 Oct 2023 21:16:56 -0400 Subject: [PATCH 1/3] feat: add option to cancel Row Detail opening - user can now prevent event and return false on `onBeforeRowDetailToggle` event to cancel opening certain Row Detail, for example the code below would block opening Row Detail when its id is `1` --- .../src/slickRowDetailView.spec.ts | 37 +++++++++++++++++-- .../src/slickRowDetailView.ts | 8 ++-- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts b/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts index 46261493f..dd5d82ca6 100644 --- a/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts +++ b/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts @@ -315,10 +315,13 @@ describe('SlickRowDetailView plugin', () => { expect(stopPropagationSpy).not.toHaveBeenCalled(); }); - it('should trigger onClick and ', () => { + it('should trigger onClick and NOT expect Row Detail to be toggled when onBeforeRowDetailToggle returns false', () => { const mockProcess = jest.fn(); const expandDetailViewSpy = jest.spyOn(plugin, 'expandDetailView'); - const beforeRowDetailToggleSpy = jest.spyOn(plugin.onBeforeRowDetailToggle, 'notify'); + const onBeforeSlickEventData = new Slick.EventData(); + jest.spyOn(onBeforeSlickEventData, 'getReturnValue').mockReturnValue(false); + const beforeRowDetailToggleSpy = jest.spyOn(plugin.onBeforeRowDetailToggle, 'notify').mockReturnValueOnce(onBeforeSlickEventData); + const afterRowDetailToggleSpy = jest.spyOn(plugin.onAfterRowDetailToggle, 'notify'); const itemMock = { id: 123, firstName: 'John', lastName: 'Doe', _collapsed: true }; const detailView = `loading...`; jest.spyOn(gridStub.getEditorLock(), 'isActive').mockReturnValue(false); @@ -337,6 +340,34 @@ describe('SlickRowDetailView plugin', () => { gridStub.onClick.notify({ row: 0, cell: 1, grid: gridStub }, clickEvent); expect(beforeRowDetailToggleSpy).toHaveBeenCalled(); + expect(afterRowDetailToggleSpy).not.toHaveBeenCalled(); + expect(expandDetailViewSpy).not.toHaveBeenCalled(); + }); + + it('should trigger onClick and expect Row Detail to be toggled', () => { + const mockProcess = jest.fn(); + const expandDetailViewSpy = jest.spyOn(plugin, 'expandDetailView'); + const beforeRowDetailToggleSpy = jest.spyOn(plugin.onBeforeRowDetailToggle, 'notify'); + const afterRowDetailToggleSpy = jest.spyOn(plugin.onAfterRowDetailToggle, 'notify'); + const itemMock = { id: 123, firstName: 'John', lastName: 'Doe', _collapsed: true }; + const detailView = `loading...`; + jest.spyOn(gridStub.getEditorLock(), 'isActive').mockReturnValue(false); + jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit').mockReturnValue(true); + jest.spyOn(gridStub, 'getDataItem').mockReturnValue(itemMock); + jest.spyOn(gridStub, 'getColumns').mockReturnValue(mockColumns); + jest.spyOn(gridStub, 'getOptions').mockReturnValue({ ...gridOptionsMock, rowDetailView: { process: mockProcess, columnIndexPosition: 0, useRowClick: true, maxRows: 2, panelRows: 2 } as any }); + + plugin.init(gridStub); + plugin.onAsyncResponse.notify({ item: itemMock, itemDetail: itemMock, detailView, }, new Slick.EventData()); + + const clickEvent = new Event('click'); + Object.defineProperty(clickEvent, 'target', { writable: true, configurable: true, value: document.createElement('div') }); + Object.defineProperty(clickEvent, 'isPropagationStopped', { writable: true, configurable: true, value: jest.fn() }); + Object.defineProperty(clickEvent, 'isImmediatePropagationStopped', { writable: true, configurable: true, value: jest.fn() }); + gridStub.onClick.notify({ row: 0, cell: 1, grid: gridStub }, clickEvent); + + expect(beforeRowDetailToggleSpy).toHaveBeenCalled(); + expect(afterRowDetailToggleSpy).toHaveBeenCalled(); expect(expandDetailViewSpy).toHaveBeenCalledWith({ _collapsed: false, _detailContent: undefined, _detailViewLoaded: true, _height: 75, _sizePadding: 3, firstName: 'John', id: 123, lastName: 'Doe' @@ -419,7 +450,7 @@ describe('SlickRowDetailView plugin', () => { _collapsed: true, _detailViewLoaded: true, _sizePadding: 0, _height: 150, _detailContent: 'loading...', id: 123, firstName: 'John', lastName: 'Doe', } - }); + }, expect.anything(), expect.anything()); expect(afterRowDetailToggleSpy).toHaveBeenCalledWith({ grid: gridStub, item: { diff --git a/packages/row-detail-view-plugin/src/slickRowDetailView.ts b/packages/row-detail-view-plugin/src/slickRowDetailView.ts index 7c69bdff0..3071b72f5 100644 --- a/packages/row-detail-view-plugin/src/slickRowDetailView.ts +++ b/packages/row-detail-view-plugin/src/slickRowDetailView.ts @@ -695,10 +695,10 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV } // trigger an event before toggling - this.onBeforeRowDetailToggle.notify({ - grid: this._grid, - item: dataContext - }); + // user could cancel the Row Detail opening when event is returning false + if (this.onBeforeRowDetailToggle.notify({ grid: this._grid, item: dataContext }, e, this).getReturnValue() === false) { + return; + } this.toggleRowSelection(args.row, dataContext); From 8396818240e53bc541b7a5ffe5e1a8158e46ff53 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Wed, 27 Dec 2023 01:03:07 -0500 Subject: [PATCH 2/3] feat: (re)add option to cancel Row Detail opening - I tried to implement this feature in a previous PR #1125 but that introduced a regression which is got fixed in this PR and reintroduces this feature without regressing this time. --- .../src/examples/example11.ts | 2 +- .../src/examples/example12.ts | 2 +- .../src/examples/example14.ts | 2 +- .../src/examples/example21.ts | 11 +++++++- .../src/core/__tests__/slickCore.spec.ts | 28 +++++++++++++++++++ packages/common/src/core/slickCore.ts | 9 +++++- .../__tests__/extension.service.spec.ts | 4 +++ .../src/slickRowDetailView.spec.ts | 6 ++-- .../src/slickRowDetailView.ts | 6 ++-- 9 files changed, 60 insertions(+), 10 deletions(-) diff --git a/examples/vite-demo-vanilla-bundle/src/examples/example11.ts b/examples/vite-demo-vanilla-bundle/src/examples/example11.ts index 752b5dbc9..4f67faa0c 100644 --- a/examples/vite-demo-vanilla-bundle/src/examples/example11.ts +++ b/examples/vite-demo-vanilla-bundle/src/examples/example11.ts @@ -47,7 +47,7 @@ const myCustomTitleValidator = (value) => { }; const customEditableInputFormatter = (_row, _cell, value, columnDef, _dataContext, grid) => { - const gridOptions = grid?.getOptions() ?? {}; + const gridOptions = grid.getOptions(); const isEditableItem = gridOptions.editable && columnDef.editor; value = (value === null || value === undefined) ? '' : value; return isEditableItem ? { html: value, addClasses: 'editable-field', toolTip: 'Click to Edit' } : value; diff --git a/examples/vite-demo-vanilla-bundle/src/examples/example12.ts b/examples/vite-demo-vanilla-bundle/src/examples/example12.ts index a07b89904..8b7d413b4 100644 --- a/examples/vite-demo-vanilla-bundle/src/examples/example12.ts +++ b/examples/vite-demo-vanilla-bundle/src/examples/example12.ts @@ -48,7 +48,7 @@ const myCustomTitleValidator = (value, args) => { * @returns {boolean} isEditable */ function checkItemIsEditable(dataContext, columnDef, grid) { - const gridOptions = grid && grid.getOptions && grid.getOptions(); + const gridOptions = grid.getOptions(); const hasEditor = columnDef.editor; const isGridEditable = gridOptions.editable; let isEditable = (isGridEditable && hasEditor); diff --git a/examples/vite-demo-vanilla-bundle/src/examples/example14.ts b/examples/vite-demo-vanilla-bundle/src/examples/example14.ts index 43df0a633..c83798b08 100644 --- a/examples/vite-demo-vanilla-bundle/src/examples/example14.ts +++ b/examples/vite-demo-vanilla-bundle/src/examples/example14.ts @@ -49,7 +49,7 @@ const myCustomTitleValidator = (value) => { * @returns {boolean} isEditable */ function checkItemIsEditable(dataContext, columnDef, grid) { - const gridOptions = grid && grid.getOptions && grid.getOptions(); + const gridOptions = grid.getOptions(); const hasEditor = columnDef.editor; const isGridEditable = gridOptions.editable; let isEditable = (isGridEditable && hasEditor); diff --git a/examples/vite-demo-vanilla-bundle/src/examples/example21.ts b/examples/vite-demo-vanilla-bundle/src/examples/example21.ts index cb8ce2ff6..99fa4d590 100644 --- a/examples/vite-demo-vanilla-bundle/src/examples/example21.ts +++ b/examples/vite-demo-vanilla-bundle/src/examples/example21.ts @@ -81,7 +81,6 @@ export default class Example21 { autoResize: { container: '.demo-container', }, - enableCellNavigation: true, enableColumnReorder: true, enableFiltering: true, enableRowDetailView: true, @@ -110,6 +109,7 @@ export default class Example21 { } changeEditableGrid() { + this.rowDetail.collapseAll(); this.rowDetail.addonOptions.useRowClick = false; this.gridOptions.autoCommitEdit = !this.gridOptions.autoCommitEdit; this.sgb.slickGrid?.setOptions({ @@ -133,6 +133,15 @@ export default class Example21 { } addRowDetailEventHandlers() { + this.rowDetail.onBeforeRowDetailToggle.subscribe((e, args) => { + // you coud cancel opening certain rows + // if (args.item.id === 1) { + // e.preventDefault(); + // return false; + // } + console.log('before toggling row detail', args.item); + }); + this._eventHandler.subscribe(this.rowDetail.onAfterRowDetailToggle, (_e, args) => { console.log('after toggling row detail', args.item); if (args.item._collapsed) { diff --git a/packages/common/src/core/__tests__/slickCore.spec.ts b/packages/common/src/core/__tests__/slickCore.spec.ts index c1b93fd41..ef02b237e 100644 --- a/packages/common/src/core/__tests__/slickCore.spec.ts +++ b/packages/common/src/core/__tests__/slickCore.spec.ts @@ -76,6 +76,16 @@ describe('SlickCore file', () => { ed.addReturnValue(false); expect(ed.getReturnValue()).toBe('last value'); }); + + it('should be able to reset value returned', () => { + const ed = new SlickEventData(); + ed.addReturnValue('last value'); + + expect(ed.getReturnValue()).toBe('last value'); + + ed.resetReturnValue(); + expect(ed.getReturnValue()).toBeUndefined(); + }); }); describe('SlickEvent class', () => { @@ -92,6 +102,24 @@ describe('SlickCore file', () => { expect(onClick.subscriberCount).toBe(1); }); + it('should be able to call notify on SlickEventData and ignore any previous value', () => { + const spy1 = jest.fn(); + const spy2 = jest.fn(); + const ed = new SlickEventData(); + const onClick = new SlickEvent('onClick'); + const scope = { onClick }; + const resetValSpy = jest.spyOn(ed, 'resetReturnValue'); + onClick.subscribe(spy1); + onClick.subscribe(spy2); + + expect(onClick.subscriberCount).toBe(2); + + onClick.notify({ hello: 'world' }, ed, scope, true); + + expect(spy1).toHaveBeenCalledWith(ed, { hello: 'world' }); + expect(resetValSpy).toHaveBeenCalled(); + }); + it('should be able to subscribe to an event, call notify() and all subscribers to receive what was sent', () => { const spy1 = jest.fn(); const spy2 = jest.fn(); diff --git a/packages/common/src/core/slickCore.ts b/packages/common/src/core/slickCore.ts index d809a1fef..43e21e51f 100644 --- a/packages/common/src/core/slickCore.ts +++ b/packages/common/src/core/slickCore.ts @@ -117,6 +117,10 @@ export class SlickEventData { getArguments() { return this._arguments; } + + resetReturnValue() { + this.returnValue = undefined; + } } /** @@ -174,8 +178,11 @@ export class SlickEvent { * @param {Object} [scope] - The scope ("this") within which the handler will be executed. * If not specified, the scope will be set to the Event instance. */ - notify(args: ArgType, evt?: SlickEventData | Event | MergeTypes | null, scope?: any) { + notify(args: ArgType, evt?: SlickEventData | Event | MergeTypes | null, scope?: any, ignorePrevEventDataValue = false) { const sed = evt instanceof SlickEventData ? evt : new SlickEventData(evt, args); + if (ignorePrevEventDataValue) { + sed.resetReturnValue(); + } scope = scope || this; for (let i = 0; i < this._handlers.length && !(sed.isPropagationStopped() || sed.isImmediatePropagationStopped()); i++) { diff --git a/packages/common/src/services/__tests__/extension.service.spec.ts b/packages/common/src/services/__tests__/extension.service.spec.ts index 4fa3fe0df..e723902c4 100644 --- a/packages/common/src/services/__tests__/extension.service.spec.ts +++ b/packages/common/src/services/__tests__/extension.service.spec.ts @@ -384,6 +384,7 @@ describe('ExtensionService', () => { const gridOptionsMock = { enableCheckboxSelector: true } as GridOption; const extCreateSpy = jest.spyOn(mockCheckboxSelectColumn, 'create').mockReturnValue(mockCheckboxSelectColumn); const gridSpy = jest.spyOn(SharedService.prototype, 'gridOptions', 'get').mockReturnValue(gridOptionsMock); + const rowSelectionSpy = jest.spyOn(SlickRowSelectionModel.prototype, 'constructor' as any); service.createExtensionsBeforeGridCreation(columnsMock, gridOptionsMock); service.bindDifferentExtensions(); @@ -393,6 +394,7 @@ describe('ExtensionService', () => { expect(gridSpy).toHaveBeenCalled(); expect(extCreateSpy).toHaveBeenCalledWith(columnsMock, gridOptionsMock); expect(rowSelectionInstance).not.toBeNull(); + expect(rowSelectionSpy).toHaveBeenCalledWith({}); expect(output).toEqual({ name: ExtensionName.checkboxSelector, instance: mockCheckboxSelectColumn as unknown } as ExtensionModel); }); @@ -401,6 +403,7 @@ describe('ExtensionService', () => { const gridOptionsMock = { enableRowMoveManager: true } as GridOption; const extCreateSpy = jest.spyOn(mockRowMoveManager, 'create').mockReturnValue(mockRowMoveManager); const gridSpy = jest.spyOn(SharedService.prototype, 'gridOptions', 'get').mockReturnValue(gridOptionsMock); + const rowSelectionSpy = jest.spyOn(SlickRowSelectionModel.prototype, 'constructor' as any); service.createExtensionsBeforeGridCreation(columnsMock, gridOptionsMock); service.bindDifferentExtensions(); @@ -410,6 +413,7 @@ describe('ExtensionService', () => { expect(gridSpy).toHaveBeenCalled(); expect(extCreateSpy).toHaveBeenCalledWith(columnsMock, gridOptionsMock); expect(rowSelectionInstance).not.toBeNull(); + expect(rowSelectionSpy).toHaveBeenCalledWith({ dragToSelect: true }); expect(output).toEqual({ name: ExtensionName.rowMoveManager, instance: mockRowMoveManager as unknown } as ExtensionModel); }); diff --git a/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts b/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts index dd5d82ca6..f122dd109 100644 --- a/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts +++ b/packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts @@ -318,7 +318,7 @@ describe('SlickRowDetailView plugin', () => { it('should trigger onClick and NOT expect Row Detail to be toggled when onBeforeRowDetailToggle returns false', () => { const mockProcess = jest.fn(); const expandDetailViewSpy = jest.spyOn(plugin, 'expandDetailView'); - const onBeforeSlickEventData = new Slick.EventData(); + const onBeforeSlickEventData = new SlickEventData(); jest.spyOn(onBeforeSlickEventData, 'getReturnValue').mockReturnValue(false); const beforeRowDetailToggleSpy = jest.spyOn(plugin.onBeforeRowDetailToggle, 'notify').mockReturnValueOnce(onBeforeSlickEventData); const afterRowDetailToggleSpy = jest.spyOn(plugin.onAfterRowDetailToggle, 'notify'); @@ -358,7 +358,7 @@ describe('SlickRowDetailView plugin', () => { jest.spyOn(gridStub, 'getOptions').mockReturnValue({ ...gridOptionsMock, rowDetailView: { process: mockProcess, columnIndexPosition: 0, useRowClick: true, maxRows: 2, panelRows: 2 } as any }); plugin.init(gridStub); - plugin.onAsyncResponse.notify({ item: itemMock, itemDetail: itemMock, detailView, }, new Slick.EventData()); + plugin.onAsyncResponse.notify({ item: itemMock, itemDetail: itemMock, detailView, }, new SlickEventData()); const clickEvent = new Event('click'); Object.defineProperty(clickEvent, 'target', { writable: true, configurable: true, value: document.createElement('div') }); @@ -450,7 +450,7 @@ describe('SlickRowDetailView plugin', () => { _collapsed: true, _detailViewLoaded: true, _sizePadding: 0, _height: 150, _detailContent: 'loading...', id: 123, firstName: 'John', lastName: 'Doe', } - }, expect.anything(), expect.anything()); + }, expect.anything(), expect.anything(), true); expect(afterRowDetailToggleSpy).toHaveBeenCalledWith({ grid: gridStub, item: { diff --git a/packages/row-detail-view-plugin/src/slickRowDetailView.ts b/packages/row-detail-view-plugin/src/slickRowDetailView.ts index 3071b72f5..c86eacee5 100644 --- a/packages/row-detail-view-plugin/src/slickRowDetailView.ts +++ b/packages/row-detail-view-plugin/src/slickRowDetailView.ts @@ -1,3 +1,4 @@ +import { createDomElement, SlickEvent, SlickEventHandler, } from '@slickgrid-universal/common'; import type { Column, DOMMouseOrTouchEvent, @@ -19,7 +20,6 @@ import type { SlickEventData, UsabilityOverrideFn, } from '@slickgrid-universal/common'; -import { createDomElement, SlickEvent, SlickEventHandler, } from '@slickgrid-universal/common'; import { extend } from '@slickgrid-universal/utils'; /** @@ -683,6 +683,7 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV /** Handle mouse click event */ protected handleClick(e: DOMMouseOrTouchEvent, args: { row: number; cell: number; }) { const dataContext = this._grid.getDataItem(args.row); + if (this.checkExpandableOverride(args.row, dataContext, this._grid)) { // clicking on a row select checkbox const columnDef = this._grid.getColumns()[args.cell]; @@ -696,7 +697,8 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV // trigger an event before toggling // user could cancel the Row Detail opening when event is returning false - if (this.onBeforeRowDetailToggle.notify({ grid: this._grid, item: dataContext }, e, this).getReturnValue() === false) { + const ignorePrevEventDataValue = true; // click event might return false from Row Selection canCellBeActive() validation, we need to ignore that + if (this.onBeforeRowDetailToggle.notify({ grid: this._grid, item: dataContext }, e, this, ignorePrevEventDataValue).getReturnValue() === false) { return; } From c2c231ea3e68630ba97678f7a1f4d7c061d886a3 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Wed, 27 Dec 2023 01:05:43 -0500 Subject: [PATCH 3/3] chore: fix failing build for Cypress --- examples/vite-demo-vanilla-bundle/src/examples/example21.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/vite-demo-vanilla-bundle/src/examples/example21.ts b/examples/vite-demo-vanilla-bundle/src/examples/example21.ts index 99fa4d590..594e6df40 100644 --- a/examples/vite-demo-vanilla-bundle/src/examples/example21.ts +++ b/examples/vite-demo-vanilla-bundle/src/examples/example21.ts @@ -133,7 +133,7 @@ export default class Example21 { } addRowDetailEventHandlers() { - this.rowDetail.onBeforeRowDetailToggle.subscribe((e, args) => { + this.rowDetail.onBeforeRowDetailToggle.subscribe((_e, args) => { // you coud cancel opening certain rows // if (args.item.id === 1) { // e.preventDefault();