Skip to content
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.

Sign up for GitHub

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

Fix bad styles being cached in massive lists #527

Merged
merged 1 commit into from
Jan 6, 2017
Merged

Fix bad styles being cached in massive lists #527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions source/Grid/Grid.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { render } from '../TestUtils'
import shallowCompare from 'react-addons-shallow-compare'
import Grid, { DEFAULT_SCROLLING_RESET_TIME_INTERVAL } from './Grid'
import { SCROLL_DIRECTION_BACKWARD, SCROLL_DIRECTION_FORWARD } from './utils/getOverscanIndices'
import { DEFAULT_MAX_SCROLL_SIZE } from './utils/ScalingCellSizeAndPositionManager'

const DEFAULT_COLUMN_WIDTH = 50
const DEFAULT_HEIGHT = 100
Expand Down Expand Up @@ -1507,4 +1508,33 @@ describe('Grid', () => {
expect(cellRendererCalls[1].style.width).toEqual(50)
})
})

it('should not pull from the style cache while scrolling if there is an offset adjustment', () => {
let cellRendererCalls = []
function cellRenderer (props) {
cellRendererCalls.push(props)
}

const grid = render(getMarkup({
cellRenderer,
width: 100,
height: 100,
rowHeight: 100,
columnWidth: 100,
rowCount: DEFAULT_MAX_SCROLL_SIZE * 2 / 100, // lots of offset
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an arbitrary/odd rowCount value. What's the logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the height of the inner container needs to be over DEFAULT_MAX_SCROLL_SIZE to trigger this. Doing DEFAULT_MAX_SCROLL_SIZE / rowHeight will get us exactly enough rows to fill the container -- so I figured I might as well double it to be sure :P

I'm happy to add comments explaining it, or if you have a better suggestion for the test use that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I think the order of operations threw me a bit. (It's late here.) It's fine as is. :)

scrollTop: DEFAULT_MAX_SCROLL_SIZE
}))

simulateScroll({
grid,
scrollTop: DEFAULT_MAX_SCROLL_SIZE + 100
})

// cellRendererCalls[0] is the element at rowIndex 0
const firstProps = cellRendererCalls[1]
const secondProps = cellRendererCalls[2]

expect(cellRendererCalls.length).toEqual(3)
expect(firstProps.style).not.toBe(secondProps.style)
})
})
4 changes: 3 additions & 1 deletion source/Grid/defaultCellRangeRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ export default function defaultCellRangeRenderer ({
visibleRowIndices
}: DefaultCellRangeRendererParams) {
const renderedCells = []
const offsetAdjusted = verticalOffsetAdjustment || horizontalOffsetAdjustment
const canCacheStyle = !isScrolling || !offsetAdjusted

for (let rowIndex = rowStartIndex; rowIndex <= rowStopIndex; rowIndex++) {
let rowDatum = rowSizeAndPositionManager.getSizeAndPositionOfCell(rowIndex)
Expand All @@ -38,7 +40,7 @@ export default function defaultCellRangeRenderer ({
let style

// Cache style objects so shallow-compare doesn't re-render unnecessarily.
if (styleCache[key]) {
if (canCacheStyle && styleCache[key]) {
style = styleCache[key]
} else {
style = {
Expand Down