fix(RowDetail): Row Detail extension should work with editable grid (#…

…896)

* fix(RowDetail): Row Detail extension should work with editable grid
- this issue was brought up in a SO question that is now deleted, the main problem was that Row Detail was never tested with editable grids and was throwing an error
- while testing with editable grid, we should also collapse all row detail when clicking on an editable cell to avoid UI issues with active cell which was showing up over the row detail panel and causing UI weirdness
- also found that the SlickRowDetail wasn't always merging its internal options with a default set of options, it was only using the options defined by the user and omitting any default values which caused problem and that was caught with `useRowClick: false` that ended up not working as expected

packages/common/src/services/gridEvent.service.ts

@@ -98,7 +98,10 @@ export class GridEventService {
       // only when the grid option "autoCommitEdit" is enabled, we will make the cell active (in focus) when clicked
       // setting the cell as active as a side effect and if "autoCommitEdit" is set to false then the Editors won't save correctly
       if (gridOptions.enableCellNavigation && (!gridOptions.editable || (gridOptions.editable && gridOptions.autoCommitEdit))) {
-        grid.setActiveCell(args.row, args.cell, false, false, true);
+        try {
+          grid.setActiveCell(args.row, args.cell, false, false, true);
+        // eslint-disable-next-line @typescript-eslint/no-shadow, no-empty
+        } catch(e) {}
       }
 
       // if the column definition has a onCellClick property (a callback function), then run it

packages/common/src/styles/_variables.scss

@@ -257,6 +257,7 @@ $slick-detail-view-icon-expand-color-hover:                 darken($slick-detail
 $slick-detail-view-icon-size:                               calc(#{$slick-icon-font-size} + 2px) !default;
 $slick-detail-view-container-bgcolor:                       #f7f7f7 !default;
 $slick-detail-view-container-border:                        1px solid #c0c0c0 !default;
+$slick-detail-view-container-left:                          0 !default;
 $slick-detail-view-container-padding:                       10px !default;
 $slick-detail-view-container-z-index:                       1000 !default;
 

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

@@ -1307,6 +1307,7 @@ input.flatpickr.form-control {
     position:           absolute;
     width:              100%;
     overflow:           auto;
+    left:               var(--slick-detail-view-container-left, $slick-detail-view-container-left);
     border:             var(--slick-detail-view-container-border, $slick-detail-view-container-border);
     background-color:   var(--slick-detail-view-container-bgcolor, $slick-detail-view-container-bgcolor);
     padding:            var(--slick-detail-view-container-padding, $slick-detail-view-container-padding);

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

@@ -54,7 +54,8 @@
     "not dead"
   ],
   "dependencies": {
-    "@slickgrid-universal/common": "workspace:~"
+    "@slickgrid-universal/common": "workspace:~",
+    "@slickgrid-universal/utils": "workspace:~"
   },
   "devDependencies": {
     "cross-env": "^7.0.3",

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

@@ -42,6 +42,7 @@ const gridStub = {
   registerPlugin: jest.fn(),
   render: jest.fn(),
   updateRowCount: jest.fn(),
+  onBeforeEditCell: new Slick.Event(),
   onClick: new Slick.Event(),
   onRendered: new Slick.Event(),
   onScroll: new Slick.Event(),
@@ -97,7 +98,9 @@ describe('SlickRowDetailView plugin', () => {
       singleRowExpand: true,
       useSimpleViewportCalc: true,
       alwaysRenderColumn: true,
-      maxRows: 300
+      maxRows: 300,
+      toolTip: 'some-tooltip',
+      width: 40,
     };
     plugin.init(gridStub);
     plugin.setOptions(mockOptions);
@@ -139,6 +142,18 @@ describe('SlickRowDetailView plugin', () => {
     expect(collapseAllSpy).toHaveBeenCalled();
   });
 
+  it('should collapse all rows when "onBeforeEditCell" event is triggered', () => {
+    const collapseAllSpy = jest.spyOn(plugin, 'collapseAll');
+    jest.spyOn(gridStub, 'getOptions').mockReturnValue({ ...gridOptionsMock, rowDetailView: { collapseAllOnSort: true } as any });
+
+    plugin.init(gridStub);
+    gridStub.onBeforeEditCell.notify({ cell: undefined as any, row: undefined as any, grid: gridStub, column: {} as Column, item: {} }, new Slick.EventData(), gridStub);
+
+    expect(plugin.getExpandedRows()).toEqual([]);
+    expect(plugin.getOutOfViewportRows()).toEqual([]);
+    expect(collapseAllSpy).toHaveBeenCalled();
+  });
+
   it('should update grid row count and re-render grid when "onRowCountChanged" event is triggered', () => {
     const updateRowCountSpy = jest.spyOn(gridStub, 'updateRowCount');
     const renderSpy = jest.spyOn(gridStub, 'render');
@@ -702,7 +717,7 @@ describe('SlickRowDetailView plugin', () => {
       plugin.setOptions({ collapsedClass: 'some-collapsed' });
       plugin.expandableOverride(() => true);
       const formattedVal = plugin.getColumnDefinition().formatter!(0, 1, '', mockColumns[0], mockItem, gridStub);
-      expect(formattedVal).toBe(`<div class="expand some-collapsed"></div>`);
+      expect(formattedVal).toBe(`<div class="detailView-toggle expand some-collapsed"></div>`);
     });
 
     it('should execute formatter and expect it to return empty string and render nothing when isPadding is True', () => {
@@ -720,7 +735,7 @@ describe('SlickRowDetailView plugin', () => {
       plugin.setOptions({ expandedClass: 'some-expanded', maxRows: 2 });
       plugin.expandableOverride(() => true);
       const formattedVal = plugin.getColumnDefinition().formatter!(0, 1, '', mockColumns[0], mockItem, gridStub);
-      expect(formattedVal).toBe(`<div class="collapse some-expanded"></div></div><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>`);
+      expect(formattedVal).toBe(`<div class="detailView-toggle collapse some-expanded"></div></div><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>`);
     });
   });
 });

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

@@ -15,6 +15,7 @@ import {
   SlickRowDetailView as UniversalRowDetailView,
   UsabilityOverrideFn,
 } from '@slickgrid-universal/common';
+import { objectAssignAndExtend } from '@slickgrid-universal/utils';
 
 // using external non-typed js libraries
 declare const Slick: SlickNamespace;
@@ -130,7 +131,9 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
     }
     this._grid = grid;
     this._gridUid = grid.getUID();
-    this._addonOptions = (this.gridOptions.rowDetailView || {}) as RowDetailView;
+    if (!this._addonOptions) {
+      this._addonOptions = objectAssignAndExtend(this.gridOptions.rowDetailView, this._defaults) as RowDetailView;
+    }
     this._keyPrefix = this._addonOptions?.keyPrefix || '_';
 
     // Update the minRowBuffer so that the view doesn't disappear when it's at top of screen + the original default 3
@@ -139,6 +142,7 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
 
     this._eventHandler
       .subscribe(this._grid.onClick, this.handleClick.bind(this) as EventListener)
+      .subscribe(this._grid.onBeforeEditCell, () => this.collapseAll())
       .subscribe(this._grid.onScroll, this.handleScroll.bind(this) as EventListener);
 
     // Sort will, by default, Collapse all of the open items (unless user implements his own onSort which deals with open row and padding)
@@ -188,7 +192,7 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
       throw new Error('[Slickgrid-Universal] The Row Detail View requires options to be passed via the "rowDetailView" property of the Grid Options');
     }
 
-    this._addonOptions = { ...this._defaults, ...gridOptions.rowDetailView } as RowDetailView;
+    this._addonOptions = objectAssignAndExtend(gridOptions.rowDetailView, this._defaults) as RowDetailView;
 
     // user could override the expandable icon logic from within the options or after instantiating the plugin
     if (typeof this._addonOptions.expandableOverride === 'function') {
@@ -227,7 +231,7 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
 
   /** set or change some of the plugin options */
   setOptions(options: Partial<RowDetailViewOption>) {
-    this._addonOptions = { ... this._addonOptions, ...options };
+    this._addonOptions = objectAssignAndExtend(options, this._addonOptions);
     if (this._addonOptions?.singleRowExpand) {
       this.collapseAll();
     }
@@ -359,7 +363,7 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV
   /** Get the Column Definition of the first column dedicated to toggling the Row Detail View */
   getColumnDefinition(): Column {
     return {
-      id: this._addonOptions?.columnId ?? '_rowDetail_',
+      id: this._addonOptions?.columnId ?? this._defaults.columnId as string | number,
       field: 'sel',
       name: '',
       alwaysRenderColumn: this._addonOptions?.alwaysRenderColumn,

packages/utils/src/utils.spec.ts

@@ -6,6 +6,7 @@ import {
   arrayRemoveItemByIndex,
   deepCopy,
   deepMerge,
+  objectAssignAndExtend,
   emptyObject,
   hasData,
   isEmptyObject,
@@ -351,6 +352,95 @@ describe('Service/Utilies', () => {
     });
   });
 
+  describe('objectAssignAndExtend method', () => {
+    it('should return undefined when both inputs are undefined', () => {
+      const obj1 = undefined;
+      const obj2 = null;
+      const output = objectAssignAndExtend(obj1, obj2);
+      expect(output).toEqual(undefined);
+    });
+
+    it('should merge object even when 1st input is undefined because 2nd input is an object', () => {
+      const input1 = undefined;
+      const input2 = { firstName: 'John' };
+      const output = objectAssignAndExtend(input1, input2);
+      expect(output).toEqual({ firstName: 'John' });
+    });
+
+    it('should merge object even when 1st input is undefined because 2nd input is an object', () => {
+      const input1 = { firstName: 'John' };
+      const input2 = undefined;
+      const output = objectAssignAndExtend(input1, input2);
+      expect(output).toEqual({ firstName: 'John' });
+    });
+
+    it('should provide empty object as input and expect output object to include 2nd object', () => {
+      const input1 = {};
+      const input2 = { firstName: 'John' };
+      const output = objectAssignAndExtend(input1, input2);
+      expect(output).toEqual({ firstName: 'John' });
+    });
+
+    it('should not overwrite property when already filled with a value', () => {
+      const input1 = { firstName: 'Jane' };
+      const input2 = { firstName: { name: 'John' } };
+      const output = objectAssignAndExtend(input1, input2);
+      expect(output).toEqual({ firstName: 'Jane' });
+    });
+
+    it('should provide input object with undefined property and expect output object to return merged object from 2nd object when that one is filled', () => {
+      const input1 = { firstName: undefined };
+      const input2 = { firstName: {} };
+      const output = objectAssignAndExtend(input1, input2);
+      expect(output).toEqual({ firstName: {} });
+    });
+
+    it('should provide input object with undefined property and not expect it to overwrite property since it already has a value', () => {
+      const input1 = { firstName: { name: 'John' } };
+      const input2 = { firstName: undefined };
+      const output = objectAssignAndExtend(input1, input2);
+      expect(output).toEqual({ firstName: { name: 'John' } });
+    });
+
+    it('should merge 2 objects and expect objects to be merged with both side', () => {
+      const input1 = { a: 1, b: 1, c: { x: 1, y: 1 }, d: [1, 1] };
+      const input2 = { b: 2, c: { y: 2, z: 2 }, d: [2, 2], e: 2 };
+
+      const output = objectAssignAndExtend(input1, input2);
+      expect(output).toEqual({
+        a: 1, b: 1, c: { x: 1, y: 1, z: 2 },
+        d: [1, 1],
+        e: 2
+      });
+    });
+
+    it('should merge 3 objects and expect objects to be merged with both side', () => {
+      const input1 = { a: 1, b: 1, c: { x: 1, y: 1 }, d: [1, 1] };
+      const input2 = { b: 2, c: { y: 2, z: 2 } };
+      const input3 = { d: [2, 2], e: 2 };
+
+      const output = objectAssignAndExtend(input1, input2, input3);
+      expect(output).toEqual({
+        a: 1, b: 1, c: { x: 1, y: 1, z: 2 },
+        d: [1, 1],
+        e: 2
+      });
+    });
+
+    it('should merge 3 objects, by calling objectAssignAndExtend 2 times, and expect objects to be merged with both side', () => {
+      const input1 = { a: 1, b: 1, c: { x: 1, y: 1 }, d: [1, 1] };
+      const input2 = { b: 2, c: { y: 2, z: 2 } };
+      const input3 = { d: [2, 2], e: 2 };
+
+      const output = objectAssignAndExtend(objectAssignAndExtend(input1, input2), input3);
+      expect(output).toEqual({
+        a: 1, b: 1, c: { x: 1, y: 1, z: 2 },
+        d: [1, 1],
+        e: 2
+      });
+    });
+  });
+
   describe('emptyObject method', () => {
     it('should empty all object properties', () => {
       const obj = { firstName: 'John', address: { zip: 123456, streetNumber: '123 Belleville Blvd' } };

packages/utils/src/utils.ts

@@ -91,8 +91,9 @@ export function deepCopy(objectOrArray: any | any[]): any | any[] {
 /**
  * Performs a deep merge of objects and returns new object, it does not modify the source object, objects (immutable) and merges arrays via concatenation.
  * Also, if first argument is undefined/null but next argument is an object then it will proceed and output will be an object
- * @param {...object} objects - Objects to merge
- * @returns {object} New object with merged key/values
+ * @param {Object} target - the target object — what to apply the sources' properties to, which is returned after it is modified.
+ * @param {Object} sources - the source object(s) — objects containing the properties you want to apply.
+ * @returns {Object} The target object.
  */
 export function deepMerge(target: any, ...sources: any[]): any {
   if (!sources.length) {
@@ -133,6 +134,39 @@ export function deepMerge(target: any, ...sources: any[]): any {
   return deepMerge(target, ...sources);
 }
 
+/**
+ * This method is similar to `Object.assign` with the exception that it will also extend the object properties when filled.
+ * There's also a distinction with extend vs merge, we are only extending when the property is not filled (if it is filled then it remains untouched and will not be merged)
+ * It also applies the change directly on the target object which mutates the original object.
+ * For example using these 2 objects: obj1 = { a: 1, b: { c: 2, d: 3 }} and obj2 = { b: { d: 2, e: 3}}:
+ *   - Object.assign(obj1, obj2) => { a: 1, b: { e: 4 }}
+ *   - objectAssignAndExtend(obj1, obj2) => { a: 1, b: { c: 2, d: 3, e: 4 }
+ * @param {Object} target - the target object — what to apply the sources properties and mutate into
+ * @param {Object} sources - the source object(s) — objects containing the properties you want to apply.
+ * @returns {Object} The target object.
+ */
+export function objectAssignAndExtend(target: any, ...sources: any): any {
+  if (!sources.length || sources[0] === undefined) {
+    return target;
+  }
+  const source = sources.shift();
+
+  // when target is not an object but source is an object, then we'll assign as object
+  target = (!isObject(target) && isObject(source)) ? {} : target;
+
+  if (isObject(target) && isObject(source)) {
+    for (const key of Object.keys(source)) {
+      if (typeof source[key] === 'object' && source[key] !== null) {
+        objectAssignAndExtend(target[key], source[key]);
+      }
+      if ((target[key] === null || target[key] === undefined) && source[key] !== null && source[key] !== undefined) {
+        target[key] = source[key];
+      }
+    }
+  }
+  return objectAssignAndExtend(target, ...sources);
+}
+
 /**
  * Empty an object properties by looping through them all and deleting them
  * @param obj - input object