Skip to content

Commit 00b45eb

Browse files
author
Alain Dumesny
committed
undefined x,y position messed up grid
fix for #1017 (missing x,y breaks grid), but also a long time issue with autoPlacement=true not respecting DOM order to place items (missing x,y or zero values) (bugged me for a while, so glad to tackle it) * no longer force x,y to be integer up front (get Nan when missing), instead we detect missing and set autoPosition=true. The get fixed soon after. * fixed Utils.defaults() field assignment to check for x=undefined not just missing * fixed code that sorted elements to be created to put autoPosition items last while keeping DOM order. * added karma test cases and UI HTML sample showing complex case - to make sure it doesn't break going forward. * fixed README & others CDN locations to use https:// so we can test them locally. TODO: karma test on HTML file wasn't working, so mine isn't tested either.
1 parent 46e8d3c commit 00b45eb

File tree

8 files changed

+131
-20
lines changed

8 files changed

+131
-20
lines changed

‎README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,9 @@ Usage
6969
* Using CDN:
7070

7171
```html
72-
<link rel="stylesheet" href="//cdnjs.cloudflare.com/ajax/libs/gridstack.js/0.5.0/gridstack.min.css" />
73-
<script type="text/javascript" src='//cdnjs.cloudflare.com/ajax/libs/gridstack.js/0.5.0/gridstack.min.js'></script>
74-
<script type="text/javascript" src='//cdnjs.cloudflare.com/ajax/libs/gridstack.js/0.5.0/gridstack.jQueryUI.min.js'></script>
72+
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/gridstack.js/0.5.0/gridstack.min.css" />
73+
<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/gridstack.js/0.5.0/gridstack.min.js"></script>
74+
<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/gridstack.js/0.5.0/gridstack.jQueryUI.min.js"></script>
7575
```
7676

7777
* Using bower:

‎doc/README.md‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,14 +103,13 @@ gridstack.js API
103103

104104
## Item attributes
105105

106-
- `data-gs-x`, `data-gs-y` - element position
106+
- `data-gs-x`, `data-gs-y` - element position. Note: if one is missing this will `autoPosition` the item
107107
- `data-gs-width`, `data-gs-height` - element size
108108
- `data-gs-id`- good for quick identification (for example in change event)
109109
- `data-gs-max-width`, `data-gs-min-width`, `data-gs-max-height`, `data-gs-min-height` - element constraints
110110
- `data-gs-no-resize` - disable element resizing
111111
- `data-gs-no-move` - disable element moving
112-
- `data-gs-auto-position` - tells to ignore `data-gs-x` and `data-gs-y` attributes and to place element to the first
113-
available position
112+
- `data-gs-auto-position` - tells to ignore `data-gs-x` and `data-gs-y` attributes and to place element to the first available position. Having either one missing will also do that.
114113
- `data-gs-locked` - the widget will be locked. It means another widget wouldn't be able to move it during dragging or resizing.
115114
The widget can still be dragged or resized. You need to add `data-gs-no-resize` and `data-gs-no-move` attributes
116115
to completely lock the widget.

‎karma.conf.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ module.exports = function(config) {
2020
'node_modules/core-js/client/shim.min.js',
2121
'src/gridstack.js',
2222
'src/gridstack.jQueryUI.js',
23-
'spec/*-spec.js'
23+
'spec/*-spec.js',
24+
// 'spec/e2e/*-spec.js' issues with ReferenceError: `browser` & `element` is not defined
2425
],
2526

2627

spec/e2e/gridstack-spec.js renamed to spec/e2e/gridstack-html-spec.js

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ describe('gridstack.js with height', function() {
77
browser.get('http://localhost:8080/spec/e2e/html/gridstack-with-height.html');
88
});
99

10-
it('shouldn\'t throw exeption when dragging widget outside the grid', function() {
10+
it('shouldn\'t throw exception when dragging widget outside the grid', function() {
1111
var widget = element(by.id('item-1'));
1212
var gridContainer = element(by.id('grid'));
1313

@@ -22,3 +22,18 @@ describe('gridstack.js with height', function() {
2222
});
2323
});
2424
});
25+
26+
describe('grid elements with no x,y positions', function() {
27+
beforeAll(function() {
28+
browser.ignoreSynchronization = true;
29+
});
30+
31+
beforeEach(function() {
32+
browser.get('http://localhost:8080/spec/e2e/html/1017-items-no-x-y-for-autoPosition.html');
33+
});
34+
35+
it('should match positions in order 5,1,2,4,3', function() {
36+
// TBD
37+
// expect(null).not.toBeNull();
38+
});
39+
});

‎spec/e2e/html/1017-items-no-x-y-for-autoPosition.html‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
<!--
2+
grid items have no x,y position, just size. Used to come up with wrong size and overlap. https://github.com/gridstack/gridstack.js/issues/1017
3+
Now, will have element 5 (autoPosition but small x,y), 1,2,4, then 3 (too big to fit)
4+
-->
5+
<!DOCTYPE html>
6+
<html lang="en">
7+
8+
<head>
9+
<!--[if lt IE 9]>
10+
<script src="http://html5shim.googlecode.com/svn/trunk/html5.js"></script>
11+
<![endif]-->
12+
13+
<meta charset="utf-8">
14+
<meta http-equiv="X-UA-Compatible" content="IE=edge">
15+
<meta name="viewport" content="width=device-width, initial-scale=1">
16+
17+
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css">
18+
<link rel="stylesheet" href="../../../dist/gridstack.css" />
19+
20+
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js"></script>
21+
<script src="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.js"></script>
22+
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js"></script>
23+
<script src="https://cdnjs.cloudflare.com/ajax/libs/core-js/2.6.9/shim.min.js"></script>
24+
<script src="../../../src/gridstack.js"></script>
25+
<script src="../../../src/gridstack.jQueryUI.js"></script>
26+
27+
<style type="text/css">
28+
.grid-stack {
29+
margin-top: 10px;
30+
background: lightgoldenrodyellow;
31+
}
32+
33+
.grid-stack-item-content {
34+
color: #2c3e50;
35+
text-align: center;
36+
background-color: #18bc9c;
37+
}
38+
39+
.upper .grid-stack-item-content {
40+
background: blue;
41+
}
42+
</style>
43+
</head>
44+
45+
<body>
46+
<div class="grid-stack">
47+
<div class="grid-stack-item upper" data-gs-width="2" data-gs-height="2" data-gs-id="1">
48+
<div class="grid-stack-item-content">item 1</div>
49+
</div>
50+
<div class="grid-stack-item" data-gs-width="3" data-gs-height="2" data-gs-id="2">
51+
<div class="grid-stack-item-content">item 2</div>
52+
</div>
53+
<div class="grid-stack-item" data-gs-width="9" data-gs-height="1" data-gs-id="3">
54+
<div class="grid-stack-item-content">item 3 too big to fit, so next row</div>
55+
</div>
56+
<div class="grid-stack-item" data-gs-width="3" data-gs-height="1" data-gs-id="4">
57+
<div class="grid-stack-item-content">item 4</div>
58+
</div>
59+
<div class="grid-stack-item" data-gs-x="1" data-gs-y="1" data-gs-width="1" data-gs-height="1" data-gs-id="5" data-gs-auto-position="false">
60+
<div class="grid-stack-item-content">item 5 first</div>
61+
</div>
62+
</div>
63+
64+
<script type="text/javascript">
65+
$(function () {
66+
var options = {
67+
cellHeight: 80,
68+
verticalMargin: 10,
69+
float: true
70+
};
71+
$('.grid-stack').gridstack(options);
72+
});
73+
</script>
74+
</body>
75+
76+
</html>

‎spec/e2e/html/gridstack-with-height.html‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,13 @@
1010
<meta name="viewport" content="width=device-width, initial-scale=1">
1111
<title>gridstack.js tests</title>
1212

13-
<link rel="stylesheet" href="//maxcdn.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css">
13+
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css">
1414
<link rel="stylesheet" href="../../../dist/gridstack.css"/>
1515

16-
<script src="//ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js"></script>
17-
<script src="//cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.js"></script>
18-
<script src="//maxcdn.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js"></script>
19-
<script src="//cdnjs.cloudflare.com/ajax/libs/core-js/2.6.9/shim.min.js"></script>
16+
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js"></script>
17+
<script src="https://cdnjs.cloudflare.com/ajax/libs/jqueryui/1.12.1/jquery-ui.js"></script>
18+
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js"></script>
19+
<script src="https://cdnjs.cloudflare.com/ajax/libs/core-js/2.6.9/shim.min.js"></script>
2020
<script src="../../../dist/gridstack.js"></script>
2121
<script src="../../../dist/gridstack.jQueryUI.js"></script>
2222

‎spec/utils-spec.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,4 +106,17 @@ describe('gridstack utils', function() {
106106
expect(function() { utils.parseHeight('-12.5 df'); }).toThrowError('Invalid height');
107107
});
108108
});
109+
110+
describe('test defaults', function() {
111+
it('should assign missing field or undefined', function() {
112+
var src = {};
113+
expect(src).toEqual({});
114+
expect(utils.defaults(src, {x: 1, y: 2})).toEqual({x: 1, y: 2});
115+
expect(utils.defaults(src, {x: 10})).toEqual({x: 1, y: 2});
116+
src.width = undefined;
117+
expect(src).toEqual({x: 1, y: 2, width: undefined});
118+
expect(utils.defaults(src, {x: 10, width: 3})).toEqual({x: 1, y: 2, width: 3});
119+
expect(utils.defaults(src, {height: undefined})).toEqual({x: 1, y: 2, width: 3, height: undefined});
120+
});
121+
});
109122
});

‎src/gridstack.js‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@
141141

142142
sources.forEach(function(source) {
143143
for (var prop in source) {
144-
if (source.hasOwnProperty(prop) && !target.hasOwnProperty(prop)) {
144+
if (source.hasOwnProperty(prop) && (!target.hasOwnProperty(prop) || target[prop] === undefined)) {
145145
target[prop] = source[prop];
146146
}
147147
}
@@ -393,7 +393,12 @@
393393
};
394394

395395
GridStackEngine.prototype._prepareNode = function(node, resizing) {
396-
node = Utils.defaults(node || {}, {width: 1, height: 1, x: 0, y: 0});
396+
node = node || {};
397+
// if we're missing position, have grid position us automatically (before we set them to 0,0)
398+
if (node.x === undefined || node.y === undefined) {
399+
node.autoPosition = true;
400+
}
401+
node = Utils.defaults(node, {width: 1, height: 1, x: 0, y: 0});
397402

398403
node.x = parseInt('' + node.x);
399404
node.y = parseInt('' + node.y);
@@ -802,11 +807,13 @@
802807
el = $(el);
803808
elements.push({
804809
el: el,
805-
i: parseInt(el.attr('data-gs-x')) + parseInt(el.attr('data-gs-y')) * _this.opts.width
810+
// if x,y are missing (autoPosition) add them to end of list - keep their respective DOM order
811+
i: (parseInt(el.attr('data-gs-x')) || 100) +
812+
(parseInt(el.attr('data-gs-y')) || 100) * _this.opts.width
806813
});
807814
});
808-
Utils.sortBy(elements, function(x) { return x.i; }).forEach(function(i) {
809-
this._prepareElement(i.el);
815+
Utils.sortBy(elements, function(x) { return x.i; }).forEach(function(item) {
816+
this._prepareElement(item.el);
810817
}, this);
811818
}
812819

@@ -1388,8 +1395,8 @@
13881395

13891396
el.addClass(this.opts.itemClass);
13901397
var node = self.grid.addNode({
1391-
x: parseInt(el.attr('data-gs-x'), 10),
1392-
y: parseInt(el.attr('data-gs-y'), 10),
1398+
x: el.attr('data-gs-x'),
1399+
y: el.attr('data-gs-y'),
13931400
width: el.attr('data-gs-width'),
13941401
height: el.attr('data-gs-height'),
13951402
maxWidth: el.attr('data-gs-max-width'),

0 commit comments

Comments
 (0)