Skip to content

Commit

Permalink
fix: hideColumnByIds wasn't hiding columns properly (#1738)
Browse files Browse the repository at this point in the history 
* fix: hideColumnByIds wasn't hiding columns properly

* refactor: use better function name
  • Loading branch information
@ghiscoding
ghiscoding authored Nov 9, 2024
1 parent 59a47b8 commit da89db4
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 37 deletions.
12 changes: 8 additions & 4 deletions packages/common/src/extensions/__tests__/slickGridMenu.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 }));

Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions packages/common/src/extensions/extensionCommonUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
}

Expand Down
54 changes: 23 additions & 31 deletions packages/common/src/services/grid.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,10 @@ export class GridService {

if (colIndexFound >= 0) {
const visibleColumns = arrayRemoveItemByIndex<Column>(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);
}

Expand All @@ -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.executeVisibilityCommands(options, ['onHeaderMenuHideColumns'], visibleColumns);
return colIndexFound;
}
}
Expand All @@ -244,26 +237,20 @@ export class GridService {
*/
hideColumnByIds(columnIds: Array<string | number>, 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.executeVisibilityCommands(options, ['onHeaderMenuHideColumns', 'onHideColumns'], finalVisibileColumns);
}
}

Expand All @@ -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.executeVisibilityCommands(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 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();
}

// do we want to trigger an event after showing
if (options?.triggerEvent) {
eventNames.forEach(name => this.pubSubService.publish(name, { columns }));
}
}

Expand Down

0 comments on commit da89db4

Please sign in to comment.