fix(RowDetail): sort change should collapse all Row Detail (#1284)

* fix(RowDetail): sort change should collapse all Row Detail
- calling sort should collapse all Row Detail, however this was only true when triggered by clicking on column header menu, however it wasn't being triggered when called from Header Menu and Grid Menu (clear all sorting)

packages/common/src/styles/_variables.scss

@@ -254,6 +254,7 @@ $slick-detail-view-icon-expand:                             "\f055" !default;
 $slick-detail-view-icon-expand-color:                       lighten($slick-primary-color, 25%) !default;
 $slick-detail-view-icon-expand-color-hover:                 darken($slick-detail-view-icon-expand-color, 10%) !default;
 $slick-detail-view-icon-size:                               calc(#{$slick-icon-font-size} + 2px) !default;
+$slick-detail-view-icon-width:                              18px !default;
 $slick-detail-view-container-bgcolor:                       #f7f7f7 !default;
 $slick-detail-view-container-border:                        1px solid #c0c0c0 !default;
 $slick-detail-view-container-left:                          0 !default;

packages/common/src/styles/slick-plugins.scss

@@ -1279,7 +1279,6 @@ input.flatpickr.form-control {
 .slick-row {
   .detail-view-toggle {
     display: inline-block;
-    cursor: pointer;
 
     &.expand {
       display: inline-block;
@@ -1302,10 +1301,16 @@ input.flatpickr.form-control {
         content: var(--slick-detail-view-icon-collapse, $slick-detail-view-icon-collapse);
       }
     }
+    &.expand,
+    &.collapse {
+      cursor: pointer;
+    }
     &.expand:before,
     &.collapse:before {
+      display: inline-block;
       font-family: var(--slick-icon-font-family, $slick-icon-font-family);
       font-size: var(--slick-detail-view-icon-size, $slick-detail-view-icon-size);
+      width: var(--slick-detail-view-icon-width, $slick-detail-view-icon-width);
     }
   }
 

packages/row-detail-view-plugin/package.json

@@ -60,6 +60,7 @@
     "@slickgrid-universal/utils": "workspace:~"
   },
   "devDependencies": {
+    "@slickgrid-universal/event-pub-sub": "workspace:~",
     "cross-env": "^7.0.3",
     "npm-run-all2": "^6.1.1"
   }

packages/row-detail-view-plugin/src/slickRowDetailView.spec.ts

@@ -1,5 +1,6 @@
 import 'jest-extended';
-import { Column, GridOption, PubSubService, type SlickDataView, SlickEvent, SlickEventData, SlickGrid, FormatterResultWithHtml } from '@slickgrid-universal/common';
+import { Column, type FormatterResultWithHtml, GridOption, type SlickDataView, SlickEvent, SlickEventData, SlickGrid, createDomElement } from '@slickgrid-universal/common';
+import { EventPubSubService } from '@slickgrid-universal/event-pub-sub';
 
 import { SlickRowDetailView } from './slickRowDetailView';
 
@@ -48,16 +49,10 @@ const gridStub = {
   onSort: new SlickEvent(),
 } as unknown as SlickGrid;
 
-const pubSubServiceStub = {
-  publish: jest.fn(),
-  subscribe: jest.fn(),
-  unsubscribe: jest.fn(),
-  unsubscribeAll: jest.fn(),
-} as PubSubService;
-
 let mockColumns: Column[];
 
 describe('SlickRowDetailView plugin', () => {
+  let eventPubSubService: EventPubSubService;
   const divContainer = document.createElement('div');
   let plugin: SlickRowDetailView;
   const gridContainerElm = document.createElement('div');
@@ -68,7 +63,8 @@ describe('SlickRowDetailView plugin', () => {
       { id: 'firstName', name: 'First Name', field: 'firstName', width: 100 },
       { id: 'lasstName', name: 'Last Name', field: 'lasstName', width: 100 },
     ];
-    plugin = new SlickRowDetailView(pubSubServiceStub);
+    eventPubSubService = new EventPubSubService();
+    plugin = new SlickRowDetailView(eventPubSubService);
     divContainer.className = `slickgrid-container ${GRID_UID}`;
     document.body.appendChild(divContainer);
   });
@@ -134,8 +130,7 @@ describe('SlickRowDetailView plugin', () => {
     jest.spyOn(gridStub, 'getOptions').mockReturnValue({ ...gridOptionsMock, rowDetailView: { collapseAllOnSort: true } as any });
 
     plugin.init(gridStub);
-    const eventData = { ...new SlickEventData(), preventDefault: jest.fn() };
-    gridStub.onSort.notify({ sortCols: [{ columnId: mockColumns[0].id, sortCol: mockColumns[0], sortAsc: true }], multiColumnSort: true, previousSortColumns: [], grid: gridStub }, eventData as any, gridStub);
+    eventPubSubService.publish('onSortChanged', {});
 
     expect(plugin.getExpandedRows()).toEqual([]);
     expect(plugin.getOutOfViewportRows()).toEqual([]);
@@ -201,7 +196,7 @@ describe('SlickRowDetailView plugin', () => {
   });
 
   it('should add the Row Detail to the column definitions at index when calling "create" without specifying position', () => {
-    const pubSubSpy = jest.spyOn(pubSubServiceStub, 'publish');
+    const pubSubSpy = jest.spyOn(eventPubSubService, 'publish');
     const processMock = jest.fn();
     const overrideMock = jest.fn();
     const rowDetailColumnMock = {
@@ -255,7 +250,7 @@ describe('SlickRowDetailView plugin', () => {
     expect(updateItemSpy).not.toHaveBeenCalled();
   });
 
-  it('should trigger "onAsyncResponse" with Row Detail from post template when no detailView is provided and expect "updateItem" from DataView to be called with new template & data', () => {
+  it('should trigger "onAsyncResponse" with Row Detail from post template from HTML string when no detailView is provided and expect "updateItem" from DataView to be called with new template & data', () => {
     const updateItemSpy = jest.spyOn(dataviewStub, 'updateItem');
     const asyncEndUpdateSpy = jest.spyOn(plugin.onAsyncEndUpdate, 'notify');
     const itemMock = { id: 123, firstName: 'John', lastName: 'Doe' };
@@ -269,6 +264,20 @@ describe('SlickRowDetailView plugin', () => {
     expect(asyncEndUpdateSpy).toHaveBeenCalledWith({ grid: gridStub, item: itemMock, itemDetail: { _detailContent: '<span>Post 123</span>', _detailViewLoaded: true, id: 123, firstName: 'John', lastName: 'Doe' } });
   });
 
+  it('should trigger "onAsyncResponse" with Row Detail from post template with HTML Element when no detailView is provided and expect "updateItem" from DataView to be called with new template & data', () => {
+    const updateItemSpy = jest.spyOn(dataviewStub, 'updateItem');
+    const asyncEndUpdateSpy = jest.spyOn(plugin.onAsyncEndUpdate, 'notify');
+    const itemMock = { id: 123, firstName: 'John', lastName: 'Doe' };
+    const postViewMock = (item) => createDomElement('span', { textContent: `Post ${item.id}` });
+    jest.spyOn(gridStub, 'getOptions').mockReturnValue({ ...gridOptionsMock, rowDetailView: { postTemplate: postViewMock } as any });
+
+    plugin.init(gridStub);
+    plugin.onAsyncResponse.notify({ item: itemMock, itemDetail: itemMock, }, new SlickEventData());
+
+    expect(updateItemSpy).toHaveBeenCalledWith(123, { _detailContent: createDomElement('span', { textContent: 'Post 123' }), _detailViewLoaded: true, id: 123, firstName: 'John', lastName: 'Doe' });
+    expect(asyncEndUpdateSpy).toHaveBeenCalledWith({ grid: gridStub, item: itemMock, itemDetail: { _detailContent: createDomElement('span', { textContent: 'Post 123' }), _detailViewLoaded: true, id: 123, firstName: 'John', lastName: 'Doe' } });
+  });
+
   it('should trigger "onAsyncResponse" with Row Detail template when detailView is provided and expect "updateItem" from DataView to be called with new template & data', () => {
     const updateItemSpy = jest.spyOn(dataviewStub, 'updateItem');
     const asyncEndUpdateSpy = jest.spyOn(plugin.onAsyncEndUpdate, 'notify');
@@ -729,14 +738,24 @@ describe('SlickRowDetailView plugin', () => {
       expect(formattedVal).toBe(``);
     });
 
-    it('should execute formatter and expect it to return empty string and render nothing when isPadding is True', () => {
-      const mockItem = { id: 123, firstName: 'John', lastName: 'Doe', _collapsed: false, _isPadding: false, _sizePadding: 5 };
+    it('should execute formatter and expect it to render detail content from HTML string', () => {
+      const mockItem = { id: 123, firstName: 'John', lastName: 'Doe', _collapsed: false, _isPadding: false, _sizePadding: 5, _detailContent: `<div>Loading...</div>` };
+      plugin.init(gridStub);
+      plugin.setOptions({ expandedClass: 'some-expanded', maxRows: 2 });
+      plugin.expandableOverride(() => true);
+      const formattedVal = plugin.getColumnDefinition().formatter!(0, 1, '', mockColumns[0], mockItem, gridStub);
+      expect(((formattedVal as FormatterResultWithHtml).html as HTMLElement).outerHTML).toBe(`<div class="detailView-toggle collapse some-expanded"></div>`);
+      expect((formattedVal as FormatterResultWithHtml).insertElementAfterTarget!.outerHTML).toBe(`<div class=\"dynamic-cell-detail cellDetailView_123\" style=\"height: 50px; top: 25px;\"><div class=\"detail-container detailViewContainer_123\"><div class=\"innerDetailView_123\"><div>Loading...</div></div></div></div>`);
+    });
+
+    it('should execute formatter and expect it to render detail content from HTML Element', () => {
+      const mockItem = { id: 123, firstName: 'John', lastName: 'Doe', _collapsed: false, _isPadding: false, _sizePadding: 5, _detailContent: createDomElement('div', { textContent: 'Loading...' }) };
       plugin.init(gridStub);
       plugin.setOptions({ expandedClass: 'some-expanded', maxRows: 2 });
       plugin.expandableOverride(() => true);
       const formattedVal = plugin.getColumnDefinition().formatter!(0, 1, '', mockColumns[0], mockItem, gridStub);
       expect(((formattedVal as FormatterResultWithHtml).html as HTMLElement).outerHTML).toBe(`<div class="detailView-toggle collapse some-expanded"></div>`);
-      expect((formattedVal as FormatterResultWithHtml).insertElementAfterTarget!.outerHTML).toBe(`<div class=\"dynamic-cell-detail cellDetailView_123\" style=\"height: 50px; top: 25px;\"><div class=\"detail-container detailViewContainer_123\"><div class=\"innerDetailView_123\">undefined</div></div></div>`);
+      expect((formattedVal as FormatterResultWithHtml).insertElementAfterTarget!.outerHTML).toBe(`<div class=\"dynamic-cell-detail cellDetailView_123\" style=\"height: 50px; top: 25px;\"><div class=\"detail-container detailViewContainer_123\"><div class=\"innerDetailView_123\"><div>Loading...</div></div></div></div>`);
     });
   });
 });

packages/row-detail-view-plugin/src/slickRowDetailView.ts

@@ -155,7 +155,8 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
 
     // Sort will, by default, Collapse all of the open items (unless user implements his own onSort which deals with open row and padding)
     if (this._addonOptions.collapseAllOnSort) {
-      this._eventHandler.subscribe(this._grid.onSort, this.collapseAll.bind(this));
+      // sort event can be triggered by column header click or from header menu
+      this.pubSubService.subscribe('onSortChanged', () => this.collapseAll());
       this._expandedRows = [];
       this._rowIdsOutOfViewport = [];
     }
@@ -605,7 +606,7 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
   }
 
   /** The Formatter of the toggling icon of the Row Detail */
-  protected detailSelectionFormatter(row: number, cell: number, value: any, columnDef: Column, dataContext: any, grid: SlickGrid): FormatterResultWithHtml | HTMLElement | '' {
+  protected detailSelectionFormatter(row: number, _cell: number, _val: any, _colDef: Column, dataContext: any, grid: SlickGrid): FormatterResultWithHtml | HTMLElement | '' {
     if (!this.checkExpandableOverride(row, dataContext, grid)) {
       return '';
     } else {
@@ -648,13 +649,17 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
         });
         const innerContainerElm = createDomElement('div', { className: `detail-container detailViewContainer_${dataContext[this._dataViewIdProperty]}` });
         const innerDetailViewElm = createDomElement('div', { className: `innerDetailView_${dataContext[this._dataViewIdProperty]}` });
-        innerDetailViewElm.innerHTML = this._grid.sanitizeHtmlString(dataContext[`${this._keyPrefix}detailContent`]);
+        if (dataContext[`${this._keyPrefix}detailContent`] instanceof HTMLElement) {
+          innerDetailViewElm.appendChild(dataContext[`${this._keyPrefix}detailContent`]);
+        } else {
+          innerDetailViewElm.innerHTML = this._grid.sanitizeHtmlString(dataContext[`${this._keyPrefix}detailContent`]);
+        }
 
         innerContainerElm.appendChild(innerDetailViewElm);
         cellDetailContainerElm.appendChild(innerContainerElm);
 
         const result: FormatterResultWithHtml = {
-          html: createDomElement('div', { className: expandedClasses }),
+          html: createDomElement('div', { className: expandedClasses.trim() }),
           insertElementAfterTarget: cellDetailContainerElm,
         };