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.

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 invalid rendering in scroll synced list with height/width exceeding browser limit #727

Merged
merged 3 commits into from
Jul 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ dist
node_modules
npm-debug.log
styles.css
.vscode
2 changes: 1 addition & 1 deletion source/Collection/CollectionView.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import getScrollbarSize from 'dom-helpers/util/scrollbarSize'
// @TODO Merge Collection and CollectionView

/**
* Specifies the number of miliseconds during which to disable pointer events while a scroll is in progress.
* Specifies the number of milliseconds during which to disable pointer events while a scroll is in progress.
* This improves performance and makes scrolling smoother.
*/
const IS_SCROLLING_TIMEOUT = 150
Expand Down
37 changes: 37 additions & 0 deletions source/Grid/Grid.jest.js
Original file line number Diff line number Diff line change
Expand Up @@ -1355,6 +1355,43 @@ describe('Grid', () => {
])
})

it('should not cache cells if the offsets are not adjusted', () => {
const cellRendererCalls = []
function cellRenderer ({ columnIndex, key, rowIndex, style }) {
cellRendererCalls.push({ columnIndex, rowIndex })
return defaultCellRenderer({ columnIndex, key, rowIndex, style })
}
const props = {
cellRenderer,
columnWidth: 100,
height: 40,
rowHeight: 20,
rowCount: 100000,
scrollToRow: 0,
width: 100
}

render(getMarkup({
...props,
scrollToRow: 0
}))
expect(cellRendererCalls).toEqual([
{ columnIndex: 0, rowIndex: 0 },
{ columnIndex: 0, rowIndex: 1 }
])

cellRendererCalls.splice(0)

render(getMarkup({
...props,
scrollToRow: 1
}))
expect(cellRendererCalls).toEqual([
{ columnIndex: 0, rowIndex: 0 },
{ columnIndex: 0, rowIndex: 1 }
])
})

it('should cache a cell once it has been rendered while scrolling', () => {
const cellRendererCalls = []
function cellRenderer ({ columnIndex, key, rowIndex, style }) {
Expand Down
4 changes: 2 additions & 2 deletions source/Grid/Grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import defaultCellRangeRenderer from './defaultCellRangeRenderer'
import scrollbarSize from 'dom-helpers/util/scrollbarSize'

/**
* Specifies the number of miliseconds during which to disable pointer events while a scroll is in progress.
* Specifies the number of milliseconds during which to disable pointer events while a scroll is in progress.
* This improves performance and makes scrolling smoother.
*/
export const DEFAULT_SCROLLING_RESET_TIME_INTERVAL = 150
Expand Down Expand Up @@ -587,7 +587,7 @@ export default class Grid extends PureComponent {

// Special case where the previous size was 0:
// In this case we don't show any windowed cells at all.
// So we should always recalculate offset aftward.
// So we should always recalculate offset afterwards.
const sizeJustIncreasedFromZero = (
(prevProps.width === 0 || prevProps.height === 0) &&
(height > 0 && width > 0)
Expand Down
2 changes: 1 addition & 1 deletion source/Grid/defaultCellRangeRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export default function defaultCellRangeRenderer ({
rowSizeAndPositionManager.areOffsetsAdjusted()
)

const canCacheStyle = !isScrolling || !areOffsetsAdjusted
const canCacheStyle = !isScrolling && !areOffsetsAdjusted;
Copy link
Owner

Choose a reason for hiding this comment

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

nit: This project uses standard (no semis)

Copy link
Author

Choose a reason for hiding this comment

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

whoops! will update


for (let rowIndex = rowStartIndex; rowIndex <= rowStopIndex; rowIndex++) {
let rowDatum = rowSizeAndPositionManager.getSizeAndPositionOfCell(rowIndex)
Expand Down
2 changes: 1 addition & 1 deletion source/Grid/utils/CellSizeAndPositionManager.jest.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ describe('CellSizeAndPositionManager', () => {
expect(() => cellSizeAndPositionManager.getSizeAndPositionOfCell(100)).toThrow()
})

it('should returnt he correct size and position information for the requested cell', () => {
it('should return the correct size and position information for the requested cell', () => {
const { cellSizeAndPositionManager } = getCellSizeAndPositionManager()
expect(cellSizeAndPositionManager.getSizeAndPositionOfCell(0).offset).toEqual(0)
expect(cellSizeAndPositionManager.getSizeAndPositionOfCell(0).size).toEqual(10)
Expand Down
2 changes: 1 addition & 1 deletion source/Grid/utils/CellSizeAndPositionManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export default class CellSizeAndPositionManager {

/**
* Total size of all cells being measured.
* This value will be completedly estimated initially.
* This value will be completely estimated initially.
* As cells as measured the estimate will be updated.
*/
getTotalSize (): number {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('ScalingCellSizeAndPositionManager', () => {
}

describe('_getOffsetPercentage', () => {
it('should eturn the correct offset fraction', () => {
it('should return the correct offset fraction', () => {
var expectations = [
{ offset: 0, expectedOffsetPercentage: 0 },
{ offset: 35, expectedOffsetPercentage: 0.5 },
Expand Down
2 changes: 1 addition & 1 deletion source/Grid/utils/ScalingCellSizeAndPositionManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default class ScalingCellSizeAndPositionManager {

/**
* Number of pixels a cell at the given position (offset) should be shifted in order to fit within the scaled container.
* The offset passed to this function is scalled (safe) as well.
* The offset passed to this function is scaled (safe) as well.
*/
getOffsetAdjustment ({
containerSize,
Expand Down