collision: row / maxRow drag 2 grids

adumesny · adumesny · commit 5bc3a425236e · 2021-03-28T12:27:44.000-07:00
* fix #1687 * broken in 4.x - dragging between 2 grids with maxRow/row set * willItFit() could modify the passed in node (with new algorithm) instead of making a clone copy to check if it fits. * added a new vertical grid demo (which has known issues documented) * added unit test
diff --git a/demo/index.html b/demo/index.html
@@ -20,6 +20,7 @@ <h1>Demos</h1>
     <li><a href="serialization.html">Serialization</a></li>
     <li><a href="static.html">Static</a></li>
     <li><a href="two.html">Two grids</a></li>
+    <li><a href="two_vertical.html">Two grids Vertical</a></li>
     <li><a href="mobile.html">Mobile touch (JQ)</a></li>
     <li><a href="vue3js.html">Vue3.js</a></li>
     <li><a href="vue2js.html">Vue2.js</a></li>
diff --git a/demo/two_vertical.html b/demo/two_vertical.html
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+  <meta charset="utf-8">
+  <meta http-equiv="X-UA-Compatible" content="IE=edge">
+  <meta name="viewport" content="width=device-width, initial-scale=1">
+  <title>Two vertical grids demo</title>
+  <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css">
+  <link rel="stylesheet" href="demo.css"/>
+  <script src="../dist/gridstack-h5.js"></script>
+</head>
+<body>
+  <div class="container-fluid">
+    <h1>Two vertical grids demo - with maxRow</h1>
+    <p>special care is needed to prevent top grid from growing and causing shifts while you are dragging (which is a know issue).<br>
+    You can either set a fix row, or have enough padding on a parent div to allow for an extra row to be created as needed), or....</p>
+    <div class="grid-stack" id="grid1"></div>
+    <br>
+    <div class="grid-stack" id="grid2"></div>
+  </div>
+  <script src="events.js"></script>
+  <script type="text/javascript">
+    let opts = {
+      row: 1, // set min and max row to freeze the height
+      dragOut: true,
+      acceptWidgets: true
+    }
+    GridStack.init(opts, document.getElementById('grid1'))
+      .load([{x:0, y:0, content: '0'}, {x:1, y:0, content: '1'}]);
+    GridStack.init(opts, document.getElementById('grid2'))
+      .load([{x:0, y:0, content: '2'}, {x:2, y:0, content: '3'}]);
+  </script>
+</body>
+</html>
diff --git a/doc/CHANGES.md b/doc/CHANGES.md
@@ -54,6 +54,7 @@ Change log
 ## 4.0.2-dev
 
 - fix [#1693](https://github.com/gridstack/gridstack.js/issues/1693) `load` after `init()` broken in 4.x
+- fix [#1687](https://github.com/gridstack/gridstack.js/issues/1687) drag between 2 grids with `row / maxRow` broken in 4.x
 
 ## 4.0.2 (2021-3-27)
 
diff --git a/spec/gridstack-spec.ts b/spec/gridstack-spec.ts
@@ -1,4 +1,4 @@
-import { GridStack } from '../src/gridstack';
+import { GridStack, GridStackNode } from '../src/gridstack';
 import { GridStackDD } from '../src/gridstack-dd'; // html5 vs Jquery set when including all file above
 import { Utils } from '../src/utils';
 
@@ -585,6 +585,14 @@ describe('gridstack', function() {
       expect(grid.willItFit({x:0, y:0, w:12, h:1})).toBe(true);
       expect(grid.willItFit({x:0, y:0, w:12, h:2})).toBe(false);
     });
+    it('willItFit() not modifying node #1687', function() {
+      // default 4x2 and 4x4 so anything pushing more than 1 will fail
+      let grid = GridStack.init({maxRow: 5});
+      let node: GridStackNode = {x:0, y:0, w:1, h:1, _id: 1, _temporaryRemoved: true};
+      expect(grid.willItFit(node)).toBe(true);
+      expect(node._temporaryRemoved).toBe(true);
+      expect(node._id).toBe(1);
+    });
 
   });
 
diff --git a/src/gridstack-dd.ts b/src/gridstack-dd.ts
@@ -193,7 +193,7 @@ GridStack.prototype._setupAcceptWidget = function(): GridStack {
         // restore some internal fields we need after clearing them all
         node._initDD =
         node._isExternal =  // DOM needs to be re-parented on a drop
-        node._temporaryRemoved = true; // so it can be insert onDrag below
+        node._temporaryRemoved = true; // so it can be inserted onDrag below
       } else {
         node.w = w; node.h = h;
         node._temporaryRemoved = true; // so we can insert it
@@ -512,7 +512,7 @@ GridStack.prototype._leave = function(node: GridStackNode, el: GridItemHTMLEleme
   if (node._temporaryRemoved) return;
   node._temporaryRemoved = true;
 
-  this.engine.removeNode(node); // remove placeholder as well
+  this.engine.removeNode(node); // remove placeholder as well, otherwise it's a sign node is not in our list, which is a bigger issue
   node.el = node._isExternal && helper ? helper : el; // point back to real item being dragged
 
   // finally if item originally came from another grid, but left us, restore things back to prev info
diff --git a/src/gridstack-engine.ts b/src/gridstack-engine.ts
@@ -473,7 +473,10 @@ export class GridStackEngine {
   }
 
   public removeNode(node: GridStackNode, removeDOM = true, triggerEvent = false): GridStackEngine {
-    if (!this.nodes.find(n => n === node)) return; // not in our list
+    if (!this.nodes.find(n => n === node)) {
+      // TEST console.log(`Error: GridStackEngine.removeNode() node._id=${node._id} not found!`)
+      return this;
+    }
     if (triggerEvent) { // we wait until final drop to manually track removed items (rather than during drag)
       this.removedNodes.push(node);
     }
@@ -557,7 +560,8 @@ export class GridStackEngine {
       float: this.float,
       nodes: this.nodes.map(n => {return {...n}})
     });
-    clone.addNode(node);
+    let n = Utils.copyPos({}, node, true); // clone node so we don't mod any settings on it! #1687
+    clone.addNode(n);
     return clone.getRow() <= this.maxRow;
   }
 
@@ -612,7 +616,7 @@ export class GridStackEngine {
     if (typeof o.w !== 'number') { o.w = node.w; }
     if (typeof o.h !== 'number') { o.h = node.h; }
     let resizing = (node.w !== o.w || node.h !== o.h);
-    let nn: GridStackNode = {maxW: node.maxW, maxH: node.maxH, minW: node.minW, minH: node.minH};
+    let nn: GridStackNode = Utils.copyPos({}, node, true); // get min/max out first, then opt positions next
     Utils.copyPos(nn, o);
     nn = this.nodeBoundFix(nn, resizing);
     Utils.copyPos(o, nn);
diff --git a/src/utils.ts b/src/utils.ts
@@ -3,7 +3,7 @@
  * Copyright (c) 2021 Alain Dumesny - see GridStack root license
  */
 
-import { GridStackElement, GridStackNode, GridStackOptions, numberOrString, GridStackPosition } from './types';
+import { GridStackElement, GridStackNode, GridStackOptions, numberOrString, GridStackPosition, GridStackWidget } from './types';
 
 export interface HeightData {
   h: number;
@@ -219,12 +219,17 @@ export class Utils {
     return true;
   }
 
-  /* copies over b size & position */
-  static copyPos(a: GridStackPosition, b: GridStackPosition): GridStackPosition {
+  /* copies over b size & position (GridStackPosition), and possibly min/max as well */
+  static copyPos(a: GridStackWidget, b: GridStackWidget, minMax = false): GridStackWidget {
     a.x = b.x;
     a.y = b.y;
     a.w = b.w;
     a.h = b.h;
+    if (!minMax) return a;
+    if (b.minW) a.minW = b.minW;
+    if (b.minH) a.minH = b.minH;
+    if (b.maxW) a.maxW = b.maxW;
+    if (b.maxH) a.maxH = b.maxH;
     return a;
   }
 
