-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent utils.synchronizeLayoutWithChildren() from forcing components to collide #342
Conversation
Interesting. I'll have to test this. Could you please remove the built files & update the comment in the moved lines? |
… to collide We've been running into a problem where sometimes, the grid that is rendered is different than the grid we specify through `layout`. We found this could be fixed by reversing the order in which we pass our children grid items to RGL, which led us to believe there was a bug in RGL that was modifying `y`. After some debugging, we found `utils.synchronizeLayoutWithChildren()` to be the culprit. As an example, here's the layout we were providing: ```javascript [ { x: 0, y: 1, w: 4, h: 2 }, { x: 0, y: 0, w: 4, h: 1 } ]; ``` When rendered, the first item should visually be below the second. However, since `synchronizeLayoutWithChildren` synchronizes the layout in the order it is provided, it was updating the `y` property of the first item to be `Math.min(bottom(layout), g.y),`, which would end up equaling 0. This meant that (when `static` was `false`), RGL would eventually recognize the new collision and update the second item to be visually below the first, by setting its `y` value to 2. This was resulting in the grid looking like this: ```javascript [ { x: 0, y: 0, w: 4, h: 2 }, { x: 0, y: 2, w: 4, h: 1 } ]; ``` My proposed change moves this logic down into the `utils.compactItem()` function, since the layout has been sorted by the time this function is called. Because we're now iterating through the items after they've been sorted, this is no longer a problem and the correct gridd is rendered.
@STRML You bet-- just force pushed those changes. Let me know if you need any more clarification or steps to reproduce. |
For more context, this is how I went about temporarily fixing this in our stack: diff --git a/js/Grid.js b/js/Grid.js
index 2ef7028..2a7d79d 100644
--- a/js/Grid.js
+++ b/js/Grid.js
@@ -2,7 +2,10 @@ import React from 'react';
import debounce from 'debounce';
import deepmerge from 'deepmerge';
-import ReactGridLayout, { WidthProvider as widthProvider } from 'react-grid-layout';
+import ReactGridLayout, {
+ utils as gridUtils,
+ WidthProvider as widthProvider,
+} from 'react-grid-layout';
import helpers from './helpers';
import { getLayout } from './configs';
@@ -41,6 +44,19 @@ class Grid extends React.Component {
window.dispatchEvent(event);
}
+ sortComponents(components) {
+ let layout = components.map(c => {
+ const item = c.props['data-grid'];
+ item.component = c;
+
+ return item;
+ });
+
+ layout = gridUtils.sortLayoutItemsByRowCol(layout);
+
+ return layout.map(c => c.component);
+ }
+
renderGridItem(component) {
let layout = component.props.layout || {};
const type = this.formatComponentType(component.props.type);
@@ -106,10 +122,13 @@ class Grid extends React.Component {
}
render() {
- const components = this.props.manifest.components
+ let components = this.props.manifest.components
.map(r => this.renderComponent(r))
.map(c => this.renderGridItem(c));
+ // see https://github.com/STRML/react-grid-layout/pull/342
+ components = this.sortComponents(components);
+
const moveComponent = layout => {
layout = layout.map(component => (
{ |
Thanks. I've been able to replicate this, but only by using I plan to rework that in RGL v2 as the ambiguity is unfortunate. Will merge shortly. |
Merged in 75dc4f3 (adjusted commit message to note |
Hey Samuel,
We just deployed RGL to production at Reflect, and wanted to thank you for your work. 👏 Shortly after deploying we found a pretty bad bug that I wanted to surface to you.
We've been running into a problem where sometimes, the grid that is rendered is
different than the grid we specify through
layout
. We found this could befixed by reversing the order in which we pass our children grid items to RGL,
which led us to believe there was a bug in RGL that was modifying
y
.After some debugging, we found
utils.synchronizeLayoutWithChildren()
to be the culprit.
As an example, here's the layout we were providing:
When rendered, the first item should visually be below the second. However,
since
synchronizeLayoutWithChildren
synchronizes the layout in the orderit is provided, it was updating the
y
property of the first itemto be
Math.min(bottom(layout), g.y),
, which would end up equaling 0.This meant that (when
static
wasfalse
), RGL would eventually recognizethe new collision and update the second item to be visually below the first,
by setting its
y
value to 2.This was resulting in the grid looking like this:
My proposed change moves this logic down into the
utils.compactItem()
function, since the layout has been sorted by the time this function is
called. Because we're now iterating through the items after they've been
sorted, this is no longer a problem and the correct grid is rendered.