Skip to content

Commit 6d636a5

Browse files
STRMLSTRML
authored
fix(allowOverlap): clone layout properly when using (react-grid-layout#1620)
* chore(dev): fix make view-example not building properly * fix(allowOverlap): Fix uncloned layout on `allowOverlap`. Fixes react-grid-layout#1564, react-grid-layout#1606 and closes react-grid-layout#1619. Add associated tests. `allowOverlap` now implies `preventCollision` explicitly, as it always should have. * chore(RGL): minor var cleanups
1 parent a707cf7 commit 6d636a5

7 files changed

+111
-44
lines changed

‎Makefile

+3-1
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,10 @@ build-example:
3939
@$(BIN)/webpack --config webpack-examples.config.js
4040
env CONTENT_BASE="/react-grid-layout/examples/" node ./examples/generate.js
4141

42+
# Note: this doesn't hot reload, you need to re-run to update files.
43+
# TODO fix that
4244
view-example:
43-
env CONTENT_BASE="/examples/" node ./examples/generate.js
45+
env CONTENT_BASE="/react-grid-layout/examples/" node ./examples/generate.js
4446
@$(BIN)/webpack serve --config webpack-examples.config.js --progress
4547

4648
# FIXME flow is usually global

‎README.md

+3-1
Original file line numberDiff line numberDiff line change
@@ -319,10 +319,12 @@ useCSSTransforms: ?boolean = true,
319319
transformScale: ?number = 1,
320320

321321
// If true, grid can be placed one over the other.
322+
// If set, implies `preventCollision`.
322323
allowOverlap: ?boolean = false,
323324

324325
// If true, grid items won't change position when being
325-
// dragged over.
326+
// dragged over. If `allowOverlap` is still false,
327+
// this simply won't allow one to drop on an existing object.
326328
preventCollision: ?boolean = false,
327329

328330
// If true, droppable elements (with `draggable={true}` attribute)

‎lib/ReactGridLayout.jsx

+8-4
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ export default class ReactGridLayout extends React.Component<Props, State> {
252252

253253
this.setState({
254254
oldDragItem: cloneLayoutItem(l),
255-
oldLayout: this.state.layout
255+
oldLayout: layout
256256
});
257257

258258
return this.props.onDragStart(layout, l, l, null, e, node);
@@ -274,7 +274,7 @@ export default class ReactGridLayout extends React.Component<Props, State> {
274274
) => {
275275
const { oldDragItem } = this.state;
276276
let { layout } = this.state;
277-
const { cols, allowOverlap } = this.props;
277+
const { cols, allowOverlap, preventCollision } = this.props;
278278
const l = getLayoutItem(layout, i);
279279
if (!l) return;
280280

@@ -296,7 +296,7 @@ export default class ReactGridLayout extends React.Component<Props, State> {
296296
x,
297297
y,
298298
isUserAction,
299-
this.props.preventCollision,
299+
preventCollision,
300300
compactType(this.props),
301301
cols,
302302
allowOverlap
@@ -659,7 +659,11 @@ export default class ReactGridLayout extends React.Component<Props, State> {
659659
const { layout } = this.state;
660660
// This is relative to the DOM element that this event fired for.
661661
const { layerX, layerY } = e.nativeEvent;
662-
const droppingPosition = { left: layerX / transformScale, top: layerY / transformScale, e };
662+
const droppingPosition = {
663+
left: layerX / transformScale,
664+
top: layerY / transformScale,
665+
e
666+
};
663667

664668
if (!this.state.droppingDOMNode) {
665669
const positionParams: PositionParams = {

‎lib/utils.js

+26-17
Original file line numberDiff line numberDiff line change
@@ -469,16 +469,23 @@ export function moveElement(
469469
// $FlowIgnore acceptable modification of read-only array as it was recently cloned
470470
if (movingUp) sorted = sorted.reverse();
471471
const collisions = getAllCollisions(sorted, l);
472-
473-
// There was a collision; abort
474-
if (preventCollision && collisions.length) {
475-
if (!allowOverlap) {
476-
log(`Collision prevented on ${l.i}, reverting.`);
477-
l.x = oldX;
478-
l.y = oldY;
479-
l.moved = false;
480-
}
481-
return layout;
472+
const hasCollisions = collisions.length > 0;
473+
474+
// We may have collisions. We can short-circuit if we've turned off collisions or
475+
// allowed overlap.
476+
if (hasCollisions && allowOverlap) {
477+
// Easy, we don't need to resolve collisions. But we *did* change the layout,
478+
// so clone it on the way out.
479+
return cloneLayout(layout);
480+
} else if (hasCollisions && preventCollision) {
481+
// If we are preventing collision but not allowing overlap, we need to
482+
// revert the position of this element so it goes to where it came from, rather
483+
// than the user's desired location.
484+
log(`Collision prevented on ${l.i}, reverting.`);
485+
l.x = oldX;
486+
l.y = oldY;
487+
l.moved = false;
488+
return layout; // did not change so don't clone
482489
}
483490

484491
// Move each item that collides away from this element.
@@ -714,13 +721,15 @@ export function synchronizeLayoutWithChildren(
714721
} else {
715722
// Nothing provided: ensure this is added to the bottom
716723
// FIXME clone not really necessary here
717-
layout.push(cloneLayoutItem({
718-
w: 1,
719-
h: 1,
720-
x: 0,
721-
y: bottom(layout),
722-
i: String(child.key)
723-
}));
724+
layout.push(
725+
cloneLayoutItem({
726+
w: 1,
727+
h: 1,
728+
x: 0,
729+
y: bottom(layout),
730+
i: String(child.key)
731+
})
732+
);
724733
}
725734
}
726735
});

‎test/examples/19-allow-overlap.jsx

-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ export default class AllowOverlap extends React.PureComponent {
5454
layout={this.state.layout}
5555
onLayoutChange={this.onLayoutChange}
5656
useCSSTransforms={true}
57-
preventCollision={true}
5857
allowOverlap={true}
5958
{...this.props}
6059
>

‎test/spec/utils-test.js

+68-18
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ describe("moveElement", () => {
123123
layoutItem,
124124
1,
125125
2, // x, y
126-
true,
127-
true, // isUserAction, preventCollision
126+
true, // isUserAction
127+
true, // preventCollision
128128
null,
129129
2 // compactType, cols
130130
)
@@ -146,8 +146,8 @@ describe("moveElement", () => {
146146
layoutItem,
147147
1,
148148
0, // x, y
149-
true,
150-
false, // isUserAction, preventCollision
149+
true, // isUserAction
150+
false, // preventCollision
151151
"vertical",
152152
2 // compactType, cols
153153
)
@@ -172,8 +172,8 @@ describe("moveElement", () => {
172172
itemA,
173173
0,
174174
1, // x, y
175-
true,
176-
false, // isUserAction, preventCollision
175+
true, // isUserAction
176+
false, // preventCollision
177177
"vertical",
178178
10 // compactType, cols
179179
)
@@ -199,8 +199,8 @@ describe("moveElement", () => {
199199
itemA,
200200
0,
201201
2, // x, y
202-
true,
203-
false, // isUserAction, preventCollision
202+
true, // isUserAction
203+
false, // preventCollision
204204
"vertical",
205205
10 // compactType, cols
206206
)
@@ -226,8 +226,8 @@ describe("moveElement", () => {
226226
itemA,
227227
1,
228228
0, // x, y
229-
true,
230-
false, // isUserAction, preventCollision
229+
true, // isUserAction
230+
false, // preventCollision
231231
"vertical",
232232
2 // compactType, cols
233233
)
@@ -253,8 +253,8 @@ describe("moveElement", () => {
253253
itemA,
254254
2,
255255
0, // x, y
256-
true,
257-
false, // isUserAction, preventCollision
256+
true, // isUserAction
257+
false, // preventCollision
258258
"horizontal",
259259
10 // compactType, cols
260260
)
@@ -283,8 +283,8 @@ describe("moveElement", () => {
283283
itemB,
284284
1,
285285
0, // x, y
286-
true,
287-
false, // isUserAction, preventCollision
286+
true, // isUserAction
287+
false, // preventCollision
288288
"vertical",
289289
4 // compactType, cols
290290
)
@@ -315,8 +315,8 @@ describe("moveElement", () => {
315315
itemB,
316316
1,
317317
0, // x, y
318-
true,
319-
false, // isUserAction, preventCollision
318+
true, // isUserAction
319+
false, // preventCollision
320320
"vertical",
321321
4 // compactType, cols
322322
)
@@ -327,6 +327,34 @@ describe("moveElement", () => {
327327
]);
328328
});
329329

330+
it("Prevent collision", () => {
331+
const layout = [
332+
{ x: 0, y: 0, w: 1, h: 10, i: "A" },
333+
{ x: 0, y: 10, w: 1, h: 1, i: "B" },
334+
{ x: 0, y: 11, w: 1, h: 1, i: "C" }
335+
];
336+
// Move A down by 2. This will collide with B and C so
337+
// the layout should be unchanged
338+
const itemA = layout[0];
339+
const modifiedLayout = moveElement(
340+
layout,
341+
itemA,
342+
0, // x
343+
2, // y
344+
true, // isUserAction
345+
true, // preventCollision
346+
null, // compactType
347+
10 // cols
348+
);
349+
expect(Object.is(layout, modifiedLayout)).toBe(true);
350+
351+
expect(layout).toEqual([
352+
expect.objectContaining({ x: 0, y: 0, w: 1, h: 10, i: "A" }),
353+
expect.objectContaining({ x: 0, y: 10, w: 1, h: 1, i: "B" }),
354+
expect.objectContaining({ x: 0, y: 11, w: 1, h: 1, i: "C" })
355+
]);
356+
});
357+
330358
it("Allow overlapping the grid items", () => {
331359
const layout = [
332360
{ x: 0, y: 0, w: 1, h: 10, i: "A" },
@@ -341,8 +369,8 @@ describe("moveElement", () => {
341369
itemA,
342370
0,
343371
2, // x, y
344-
true,
345-
true, // isUserAction, preventCollision
372+
true, // isUserAction
373+
false, // preventCollision
346374
null,
347375
10, // compactType, cols
348376
true // allowOverlap
@@ -353,6 +381,28 @@ describe("moveElement", () => {
353381
expect.objectContaining({ x: 0, y: 11, w: 1, h: 1, i: "C" })
354382
]);
355383
});
384+
385+
it("Layout is cloned when using allowOverlap (#1606)", () => {
386+
const layout = [
387+
{ x: 0, y: 0, w: 1, h: 10, i: "A" },
388+
{ x: 0, y: 10, w: 1, h: 1, i: "B" },
389+
{ x: 0, y: 11, w: 1, h: 1, i: "C" }
390+
];
391+
// Move A down by 2. Both B and C should remain in same position
392+
const itemA = layout[0];
393+
const modifiedLayout = moveElement(
394+
layout,
395+
itemA,
396+
0,
397+
2, // x, y
398+
true, // isUserAction
399+
false, // preventCollision
400+
null,
401+
10, // compactType, cols
402+
true // allowOverlap
403+
);
404+
expect(Object.is(layout, modifiedLayout)).toBe(false);
405+
});
356406
});
357407

358408
describe("compact vertical", () => {

‎webpack-examples.config.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,13 @@ module.exports = {
4444
devServer: {
4545
compress: true,
4646
port: 4002,
47-
open: "examples/0-showcase.html",
47+
open: "/react-grid-layout/examples/0-showcase.html",
4848
client: {
4949
overlay: true
5050
},
5151
static: {
52-
directory: "."
52+
directory: ".",
53+
publicPath: "/react-grid-layout"
5354
}
5455
},
5556
resolve: {

0 commit comments

Comments
 (0)