From 0d46dafdf76df80f425f8dd3e7b340652f4742ff Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Sat, 9 Nov 2024 13:46:23 -0500 Subject: [PATCH 1/2] fix: hideColumnByIds wasn't hiding columns properly --- .../__tests__/slickGridMenu.spec.ts | 12 +++-- .../src/extensions/extensionCommonUtils.ts | 5 +- packages/common/src/services/grid.service.ts | 54 ++++++++----------- 3 files changed, 34 insertions(+), 37 deletions(-) diff --git a/packages/common/src/extensions/__tests__/slickGridMenu.spec.ts b/packages/common/src/extensions/__tests__/slickGridMenu.spec.ts index 1a1725a06..38d2ee9b5 100644 --- a/packages/common/src/extensions/__tests__/slickGridMenu.spec.ts +++ b/packages/common/src/extensions/__tests__/slickGridMenu.spec.ts @@ -90,7 +90,7 @@ describe('GridMenuControl', () => { const columnsMock: Column[] = [ { id: 'field1', field: 'field1', name: 'Field 1', width: 100, nameKey: 'TITLE' }, { id: 'field2', field: 'field2', name: 'Field 2', width: 75 }, - { id: 'field3', field: 'field3', name: 'Field 3', width: 75, columnGroup: 'Billing' }, + { id: 'field3', field: 'field3', name: 'Field 3', width: 75, columnGroup: 'Billing', excludeFromGridMenu: true }, ]; let backendUtilityService: BackendUtilityService; let extensionUtility: ExtensionUtility; @@ -681,7 +681,11 @@ describe('GridMenuControl', () => { const forceFitElm = control.menuElement!.querySelector('#slickgrid_124343-gridmenu-colpicker-forcefit') as HTMLInputElement; const inputSyncElm = control.menuElement!.querySelector('#slickgrid_124343-gridmenu-colpicker-syncresize') as HTMLInputElement; const pickerField1Elm = document.querySelector('input[type="checkbox"][data-columnid="field1"]') as HTMLInputElement; + const li2Elm = document.querySelector('.slick-column-picker-list li:nth-of-type(2)') as HTMLLIElement; + const li3Elm = document.querySelector('.slick-column-picker-list li:nth-of-type(3)') as HTMLLIElement; expect(pickerField1Elm.checked).toBe(true); + expect(li2Elm.className).not.toBe('hidden'); + expect(li3Elm.className).toBe('hidden'); pickerField1Elm.checked = false; pickerField1Elm.dispatchEvent(new Event('click', { bubbles: true, cancelable: true, composed: false })); @@ -1559,12 +1563,12 @@ describe('GridMenuControl', () => { const columnsUnorderedMock: Column[] = [ { id: 'field2', field: 'field2', name: 'Field 2', width: 75 }, { id: 'field1', field: 'field1', name: 'Titre', width: 100, nameKey: 'TITLE' }, - { id: 'field3', field: 'field3', name: 'Field 3', width: 75, columnGroup: 'Billing' }, + { id: 'field3', field: 'field3', name: 'Field 3', width: 75, columnGroup: 'Billing', excludeFromGridMenu: true }, ]; const columnsMock: Column[] = [ { id: 'field1', field: 'field1', name: 'Titre', width: 100, nameKey: 'TITLE' }, { id: 'field2', field: 'field2', name: 'Field 2', width: 75 }, - { id: 'field3', field: 'field3', name: 'Field 3', width: 75, columnGroup: 'Billing' }, + { id: 'field3', field: 'field3', name: 'Field 3', width: 75, columnGroup: 'Billing', excludeFromGridMenu: true }, ]; vi.spyOn(gridStub, 'getColumnIndex').mockReturnValue(undefined as any).mockReturnValueOnce(0).mockReturnValueOnce(1); const handlerSpy = vi.spyOn(control.eventHandler, 'subscribe'); @@ -1630,7 +1634,7 @@ describe('GridMenuControl', () => { expect(columnsMock).toEqual([ { id: 'field1', field: 'field1', name: 'Titre', width: 100, nameKey: 'TITLE' }, { id: 'field2', field: 'field2', name: 'Field 2', width: 75 }, - { id: 'field3', field: 'field3', name: 'Field 3', columnGroup: 'Billing', width: 75 }, + { id: 'field3', field: 'field3', name: 'Field 3', columnGroup: 'Billing', width: 75, excludeFromGridMenu: true }, ]); expect(control.getAllColumns()).toEqual(columnsMock); expect(control.getVisibleColumns()).toEqual(columnsMock); diff --git a/packages/common/src/extensions/extensionCommonUtils.ts b/packages/common/src/extensions/extensionCommonUtils.ts index 90ead6b85..ecab9fc9b 100644 --- a/packages/common/src/extensions/extensionCommonUtils.ts +++ b/packages/common/src/extensions/extensionCommonUtils.ts @@ -150,12 +150,13 @@ function generatePickerCheckbox(columnLiElm: HTMLLIElement, inputId: string, inp export function populateColumnPicker(this: SlickColumnPicker | SlickGridMenu, addonOptions: ColumnPickerOption | GridMenuOption): void { const context: any = this; - const menuPrefix = context instanceof SlickGridMenu ? 'gridmenu-' : ''; + const isGridMenu = context instanceof SlickGridMenu; + const menuPrefix = isGridMenu ? 'gridmenu-' : ''; for (const column of context.columns) { const columnId = column.id; const columnLiElm = document.createElement('li'); - if (column.excludeFromColumnPicker) { + if ((column.excludeFromColumnPicker && !isGridMenu) || (column.excludeFromGridMenu && isGridMenu)) { columnLiElm.className = 'hidden'; } diff --git a/packages/common/src/services/grid.service.ts b/packages/common/src/services/grid.service.ts index 6d6b89910..9f77a9b64 100644 --- a/packages/common/src/services/grid.service.ts +++ b/packages/common/src/services/grid.service.ts @@ -205,10 +205,10 @@ export class GridService { if (colIndexFound >= 0) { const visibleColumns = arrayRemoveItemByIndex(currentColumns, colIndexFound); - this.sharedService.visibleColumns = visibleColumns; // do we want to apply the new columns in the grid if (options?.applySetColumns) { + this.sharedService.visibleColumns = visibleColumns; this._grid.setColumns(visibleColumns); } @@ -222,15 +222,8 @@ export class GridService { } } - // do we want to auto-resize the columns in the grid after hidding some? most often yes - if (options?.autoResizeColumns) { - this._grid.autosizeColumns(); - } - - // do we want to trigger an event after hidding - if (options?.triggerEvent) { - this.pubSubService.publish('onHeaderMenuHideColumns', { columns: visibleColumns }); - } + // execute common grid commands when enabled + this.executeResizeTrigger(options, ['onHeaderMenuHideColumns'], visibleColumns); return colIndexFound; } } @@ -244,26 +237,20 @@ export class GridService { */ hideColumnByIds(columnIds: Array, options?: HideColumnOption): void { if (Array.isArray(columnIds)) { + const finalVisibileColumns = this._grid.getColumns().filter(c => !columnIds.includes(c.id)); options = { ...HideColumnOptionDefaults, ...options }; for (const columnId of columnIds) { // hide each column by its id but wait after the for loop to auto resize columns in the grid this.hideColumnById(columnId, { ...options, triggerEvent: false, applySetColumns: false, autoResizeColumns: false }); } - // after looping through all columns, we can apply the new columns in the grid - this._grid.setColumns(this.sharedService.visibleColumns); + // after looping through all columns, we can apply the leftover visible columns in the grid & keep shared ref + this.sharedService.visibleColumns = finalVisibileColumns; + this._grid.setColumns(finalVisibileColumns); - // do we want to auto-resize the columns in the grid after hidding some? most often yes - if (options?.autoResizeColumns) { - this._grid.autosizeColumns(); - } - - // do we want to trigger an event after hidding - if (options?.triggerEvent) { - // @deprecate `onHeaderMenuHideColumns` event, we should keep only `onHideColumns` - this.pubSubService.publish('onHeaderMenuHideColumns', { columns: this.sharedService.visibleColumns }); - this.pubSubService.publish('onHideColumns', { columns: this.sharedService.visibleColumns }); - } + // execute common grid commands when enabled + // @deprecate `onHeaderMenuHideColumns` event, we should keep only `onHideColumns` + this.executeResizeTrigger(options, ['onHeaderMenuHideColumns', 'onHideColumns'], finalVisibileColumns); } } @@ -279,15 +266,20 @@ export class GridService { this._grid.setColumns(columns); this.sharedService.visibleColumns = columns; - // do we want to auto-resize the columns in the grid after hidding some? most often yes - if (options?.autoResizeColumns) { - this._grid.autosizeColumns(); - } + // execute common grid commands when enabled + this.executeResizeTrigger(options, ['onShowColumns'], this.sharedService.visibleColumns); + } + } - // do we want to trigger an event after showing - if (options?.triggerEvent) { - this.pubSubService.publish('onShowColumns', { columns: this.sharedService.visibleColumns }); - } + protected executeResizeTrigger(options: { autoResizeColumns?: boolean; triggerEvent?: boolean; }, eventNames: string[], columns: Column[]): void { + // do we want to auto-resize the columns in the grid after hidding/showing columns? + if (options?.autoResizeColumns) { + this._grid.autosizeColumns(); + } + + // do we want to trigger an event after showing + if (options?.triggerEvent) { + eventNames.forEach(name => this.pubSubService.publish(name, { columns })); } } From e32d9f0a652767a5f3a7959629bf3f4f3ba213d0 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Sat, 9 Nov 2024 13:53:27 -0500 Subject: [PATCH 2/2] refactor: use better function name --- packages/common/src/services/grid.service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/common/src/services/grid.service.ts b/packages/common/src/services/grid.service.ts index 9f77a9b64..a0a893df6 100644 --- a/packages/common/src/services/grid.service.ts +++ b/packages/common/src/services/grid.service.ts @@ -223,7 +223,7 @@ export class GridService { } // execute common grid commands when enabled - this.executeResizeTrigger(options, ['onHeaderMenuHideColumns'], visibleColumns); + this.executeVisibilityCommands(options, ['onHeaderMenuHideColumns'], visibleColumns); return colIndexFound; } } @@ -250,7 +250,7 @@ export class GridService { // execute common grid commands when enabled // @deprecate `onHeaderMenuHideColumns` event, we should keep only `onHideColumns` - this.executeResizeTrigger(options, ['onHeaderMenuHideColumns', 'onHideColumns'], finalVisibileColumns); + this.executeVisibilityCommands(options, ['onHeaderMenuHideColumns', 'onHideColumns'], finalVisibileColumns); } } @@ -267,11 +267,11 @@ export class GridService { this.sharedService.visibleColumns = columns; // execute common grid commands when enabled - this.executeResizeTrigger(options, ['onShowColumns'], this.sharedService.visibleColumns); + this.executeVisibilityCommands(options, ['onShowColumns'], this.sharedService.visibleColumns); } } - protected executeResizeTrigger(options: { autoResizeColumns?: boolean; triggerEvent?: boolean; }, eventNames: string[], columns: Column[]): void { + protected executeVisibilityCommands(options: { autoResizeColumns?: boolean; triggerEvent?: boolean; }, eventNames: string[], columns: Column[]): void { // do we want to auto-resize the columns in the grid after hidding/showing columns? if (options?.autoResizeColumns) { this._grid.autosizeColumns();