MultiGrid (with rowHeight and columnWidth functions) resizing #552

codingbull · 2017-01-24T15:49:01Z

For a MultiGrid (wrapped by an AutoSizer) with rowHeight and columnWidth functions, grid sizes are not recalculated after window resizing. I attached an onResize function to the AutoSizer to explicitly force it via calling MultiGrid.recomputeGridSize(). In the course of debugging that, I encountered these 3 bugs.

Even after calling recomputeGridSize with these fixes, I still needed to call _maybeCalculateCachedStyles, but I wan't sure if that should always occur in the recomputeGridSize call or not.

bvaughn · 2017-01-25T02:59:17Z

source/MultiGrid/MultiGrid.js

    })
-    this._bottomRightGrid && this._bottomRightGrid.measureAllCells({


Duh, this was a stupid typo on my part. Apologies.

bvaughn · 2017-01-25T03:01:10Z

source/MultiGrid/MultiGrid.js

      fixedColumnCount !== nextProps.fixedColumnCount
    ) {
      this._leftGridWidth = null
    }

    if (
      fixedRowCount !== nextProps.fixedRowCount ||
-      rowHeight !== nextProps.rowHeight
+      rowHeight instanceof Function || rowHeight !== nextProps.rowHeight


The previous behavior here was intentional. If a function prop is passed, Grid (and so MultiGrid) don't assume they can detect when it has changed. That's because (a) things outside of the function may invalidate the size without the function prop itself changing and (b) people often define inline props like so: rowHeight={(index) => blah} which will recreate on each render. Invalidating cache is expensive so this is an intentional optimization. If a user uses a function prop, he should manually clear the cache.

That makes sense on the optimization. By what method should the cache be cleared?
Perhaps a new public method like:

clearCachedStyles() { this._topGridHeight = null; this._leftGridWidth = null; this._maybeCalculateCachedStyles(null, this.props) }

Users should call recomputeGridSize if something has happened to invalid one or more columns/rows. Grid then resets its own cache when recomputeGridSize is called.

That being said, I see your point that nothing inside of MultiGrid currently resets _leftGridWidth or _topGridHeight. I think MultiGrid should follow the precedent in Grid and reset it in recomputeGridSize.

bvaughn · 2017-01-25T03:01:34Z

source/MultiGrid/MultiGrid.js

@@ -366,6 +369,8 @@ export default class MultiGrid extends Component {
        left: 0,
        outline: 0,
        overflow: 'hidden',
+        overflowX: 'hidden',
+        overflowY: 'hidden',


Why are the long-form props needed?

I think they are needed to counteract gridStyle.overflowX and gridStyle.overflowY being set in
https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/Grid.js#L566

Roger that. We should go ahead and get rid of the short-hand form (overflow) then. It's totally redundant.

bvaughn · 2017-01-25T03:02:04Z

source/MultiGrid/MultiGrid.test.js

+        }
+      })))
+
+      multiGrid.recomputeGridSize()


I don't understand the purpose of this test. Could you explain what it's testing?

When I first switched the measureAllCells calls to recomputeGridSize, negative values were getting passed as column and row indexes, which would throw an error. This was my (poor) failing test prior to adding the comparison against the fixed row/col sizes.

I'm not sure what else to explicitly assert, but at least I can add expect(...).not.toThrowError(...);

Hm. I see.

expect(...).not.toThrowError(...) is kind of the default behavior of any testing framework though so probably not worth explicitly adding.

Perhaps I was just confused by the name of this test. What makes it different than this one for instance. You could update it to say something about recomputeGridSize specifically, since that's what it's testing, but I wonder if we couldn't do 1 better than that and assert that it actually recomputes the inner Grid sizes (and resets _leftGridWidth and _topGridHeight like we chatted about above)?

I was thinking the same thing this morning, I'll see what I can whip up.

bvaughn · 2017-01-25T03:03:53Z

For a MultiGrid (wrapped by an AutoSizer) with rowHeight and columnWidth functions, grid sizes are not recalculated after window resizing

Function props like rowHeight and columnWidth don't change when the Grid's outer size changes so I'm not sure why they are mentioned together like this. The size of a cell and the size of the Grid are different. AutoSizer only drives the size of the Grid

codingbull · 2017-01-25T04:02:45Z

Sorry, I didn't explain the setup well. Nothing wrong with AutoSizer, I just wanted to explain my usage since I didn't have a stand-alone test case to show.
I've got a rowHeight method that is returning different sizes for my fixed row based on the window size. I realize that having non-constant cell sizes is a performance hit, but I'm still getting phenomenal render performance even on mobile (so much thanks and kudos to you for that).
After a window resize, the cell was getting new position styles, but the top-left and top-right grids still have the cached styles from prior to the resize.

isSmallScreen() {
    // (fyi, IncludeMedia is just exposing my CSS media query breakpoints into the JS).
    return IncludeMedia.lessThan('md');
}
_rowHeight({rowIndex}) {
    if (rowIndex == 0) { return (this.isSmallScreen() ? 150 : 100); }
    return 40;
}

bvaughn · 2017-01-25T16:01:20Z

After a window resize, the cell was getting new position styles, but the top-left and top-right grids still have the cached styles from prior to the resize.

Thank you for clarifying! I understand now. 😁

I think this is actually caused by a combination of the typo you fixed here and what I mentioned in this comment.

Thanks for taking the time to submit this PR too! Couple of minor changes and I think it will be good to merge!

recomputeGridSize should be manually called by a user if rowHeight and/or columnWidth values are invalidated by something outside of the functions (e.g. a window resize)

codingbull · 2017-01-26T15:51:51Z

@bvaughn I think I've got a much better test now.

bvaughn

This test does look much better. Thank you!

One small change requested but otherwise looks great!

bvaughn · 2017-01-26T22:37:39Z

source/MultiGrid/MultiGrid.js

-      columnIndex,
-      rowIndex: rowIndex - fixedRowCount
+    const col = (columnIndex >= fixedColumnCount ? columnIndex - fixedColumnCount : columnIndex)
+    const row = (rowIndex >= fixedRowCount ? rowIndex - fixedRowCount : rowIndex)


Sorry, just noticed these 2 lines. This change doesn't look right. I believe these 2 lines should be reverted to the way it was before but with your correction of my recomputeGridSize typo 😄

Without that guard, I get:

TypeError: Cannot read property 'offset' of undefined at CellSizeAndPositionManager.getVisibleCellRange (CellSizeAndPositionManager.js:184) at ScalingCellSizeAndPositionManager.getVisibleCellRange (ScalingCellSizeAndPositionManager.js:150) at Grid._calculateChildrenToRender (Grid.js:544) at Grid.componentWillUpdate (Grid.js:428) at ReactCompositeComponent.js:711 at measureLifeCyclePerf (ReactCompositeComponent.js:75) at ReactCompositeComponentWrapper._performComponentUpdate (ReactCompositeComponent.js:710) at ReactCompositeComponentWrapper.updateComponent (ReactCompositeComponent.js:645) at ReactCompositeComponentWrapper.receiveComponent (ReactCompositeComponent.js:547) at Object.receiveComponent (ReactReconciler.js:125)

Instead of dealing with it in MultiGrid, perhaps a change to CellSizeAndPositionManager#resetCell instead?

resetCell (index: number): void { let guarded = Math.max(index, 0) this._lastMeasuredIndex = Math.min(this._lastMeasuredIndex, guarded - 1) }

Ah, fair enough. I think the correct fix is to add a Math.max(0, maybeNegativeIndex) in MultiGrid so that it doesn't pass negative values. As it is (with this change) we may skip cells in the bottom and/or right grids if indices > 0 and < fixed-count are passed.

bvaughn · 2017-01-26T22:37:50Z

source/MultiGrid/MultiGrid.js

@@ -380,7 +387,8 @@ export default class MultiGrid extends Component {
      this._topRightGridStyle = {
        left: this._getLeftGridWidth(props),
        outline: 0,
-        overflow: 'hidden',
+        overflowX: 'hidden',
+        overflowY: 'hidden',


Good fix 👍

bvaughn · 2017-01-27T05:43:44Z

I've merged your PR locally, after fixing that one small issue. Thanks so much, again!

bvaughn · 2017-01-27T05:44:27Z

Not sure why Github still thinks the PR is open. You can see that it's been merged here. Oh well. 😁

bvaughn · 2017-01-28T07:10:08Z

Released with 8.11.3

Gave you credit for this fix in the changelog

codingbull added 3 commits January 23, 2017 23:09

Clear cached grid sizes when cell sizes are functions

62eead0

Override all overflow styles in fixed grid quadrants

1717656

Chain recompute to quadrant grids

9de4f2b

bvaughn reviewed Jan 25, 2017

View reviewed changes

Clear cached styles only in recomputeGridSize

9fa4e60

recomputeGridSize should be manually called by a user if rowHeight and/or columnWidth values are invalidated by something outside of the functions (e.g. a window resize)

bvaughn reviewed Jan 26, 2017

View reviewed changes

bvaughn closed this Jan 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MultiGrid (with rowHeight and columnWidth functions) resizing #552

MultiGrid (with rowHeight and columnWidth functions) resizing #552

codingbull commented Jan 24, 2017

bvaughn Jan 25, 2017

bvaughn Jan 25, 2017

codingbull Jan 25, 2017

bvaughn Jan 25, 2017 •

edited

Loading

bvaughn Jan 25, 2017

codingbull Jan 25, 2017

bvaughn Jan 25, 2017

bvaughn Jan 25, 2017

codingbull Jan 25, 2017

codingbull Jan 25, 2017

bvaughn Jan 25, 2017

codingbull Jan 25, 2017

bvaughn commented Jan 25, 2017

codingbull commented Jan 25, 2017

bvaughn commented Jan 25, 2017

codingbull commented Jan 26, 2017

bvaughn left a comment

bvaughn Jan 26, 2017

codingbull Jan 27, 2017

codingbull Jan 27, 2017

bvaughn Jan 27, 2017

bvaughn Jan 26, 2017

bvaughn commented Jan 27, 2017

bvaughn commented Jan 27, 2017

bvaughn commented Jan 28, 2017

		})
		this._bottomRightGrid && this._bottomRightGrid.measureAllCells({

MultiGrid (with rowHeight and columnWidth functions) resizing #552

MultiGrid (with rowHeight and columnWidth functions) resizing #552

Conversation

codingbull commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jan 25, 2017

codingbull commented Jan 25, 2017

bvaughn commented Jan 25, 2017

codingbull commented Jan 26, 2017

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jan 27, 2017

bvaughn commented Jan 27, 2017

bvaughn commented Jan 28, 2017

bvaughn Jan 25, 2017 •

edited

Loading