Skip to content

Commit

Permalink
fix(grouping): DraggableGrouping could throw when leaving page
Browse files Browse the repository at this point in the history
- this fixes 2 issues identified in Angular-Slickgrid, ghiscoding/Angular-Slickgrid#1180
- when leaving a SPA page and going back to it, it could throw a bunch of errors. We simply need to make sure that any sortable instances are destroyed before recreating any new ones
  • Loading branch information
ghiscoding committed Jun 30, 2023
1 parent 30a0748 commit 2c2cba3
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 14 deletions.
2 changes: 1 addition & 1 deletion packages/common/src/extensions/extensionCommonUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export function handleColumnPickerItemClick(this: SlickColumnPicker | SlickGridM
context.grid.setColumns(visibleColumns);

// keep reference to the updated visible columns list
if (Array.isArray(visibleColumns) && visibleColumns.length !== context.sharedService.visibleColumns.length) {
if (!context.sharedService.visibleColumns || (Array.isArray(visibleColumns) && visibleColumns.length !== context.sharedService.visibleColumns.length)) {
context.sharedService.visibleColumns = visibleColumns;
}

Expand Down
24 changes: 11 additions & 13 deletions packages/common/src/extensions/slickDraggableGrouping.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ import { sortByFieldType } from '../sortComparers';
// using external SlickGrid JS libraries
declare const Slick: SlickNamespace;

// use global variable because "setupColumnReorder()" method is used as static
// and doesn't have access to anything inside the class and we need to dipose/destroy cleanly of all Sortable instances
// let sortableLeftInstance: SortableInstance;
// let sortableRightInstance: SortableInstance;

/**
*
* Draggable Grouping contributed by: Muthukumar Selconstasu
Expand Down Expand Up @@ -215,12 +210,7 @@ export class SlickDraggableGrouping {

/** Dispose the plugin. */
dispose() {
if (this._sortableLeftInstance?.el) {
this._sortableLeftInstance?.destroy();
}
if (this._sortableRightInstance?.el) {
this._sortableRightInstance?.destroy();
}
this.destroySortableInstances();
this.onGroupChanged.unsubscribe();
this._eventHandler.unsubscribeAll();
this.pubSubService.unsubscribeAll(this._subscriptions);
Expand All @@ -245,6 +235,15 @@ export class SlickDraggableGrouping {
}
}

destroySortableInstances() {
if (this._sortableLeftInstance?.el) {
this._sortableLeftInstance?.destroy();
}
if (this._sortableRightInstance?.el) {
this._sortableRightInstance?.destroy();
}
}

setAddonOptions(options: Partial<DraggableGroupingOption>) {
this._addonOptions = { ...this._addonOptions, ...options };
}
Expand Down Expand Up @@ -276,6 +275,7 @@ export class SlickDraggableGrouping {
* @param trigger - callback to execute when triggering a column grouping
*/
setupColumnReorder(grid: SlickGrid, headers: any, _headerColumnWidthDiff: any, setColumns: (columns: Column[]) => void, setupColumnResize: () => void, _columns: Column[], getColumnIndex: (columnId: string) => number, _uid: string, trigger: (slickEvent: SlickEvent, data?: any) => void) {
this.destroySortableInstances();
const dropzoneElm = grid.getPreHeaderPanel();
const draggablePlaceholderElm = dropzoneElm.querySelector<HTMLDivElement>('.slick-draggable-dropzone-placeholder');
const groupTogglerElm = dropzoneElm.querySelector<HTMLDivElement>('.slick-group-toggle-all');
Expand Down Expand Up @@ -545,7 +545,6 @@ export class SlickDraggableGrouping {

this._droppableInstance = Sortable.create(dropzoneElm, {
group: 'shared',
// chosenClass: 'slick-header-column-active',
ghostClass: 'slick-droppable-sortitem-hover',
draggable: '.slick-dropped-grouping',
dragoverBubble: true,
Expand All @@ -554,7 +553,6 @@ export class SlickDraggableGrouping {
if (el.getAttribute('id')?.replace(this._gridUid, '')) {
this.handleGroupByDrop(dropzoneElm, (Sortable.utils as any).clone(evt.item));
}
evt.clone.style.opacity = '.5';
el.parentNode?.removeChild(el);
},
onUpdate: () => {
Expand Down

0 comments on commit 2c2cba3

Please sign in to comment.