Skip to content

Commit

Permalink
fix(gridSystem): fix itemsPerRow precision (#892)
Browse files Browse the repository at this point in the history
* fix(gridSystem): fix `itemsPerRow` precision

- Fix `itemsPerRow()` calculation to handle edge cases related to element dimension precision
- Add tests with experimentally-determined edge cases that cause precision problems with the
previous calculation
- fixes [BUG] `VirtuosoGrid` + CSS Grid `List` component: items not rendered #889

`react-virtuoso` calculates element dimensions using `getBoundingClientRect()`. This value may be
imprecise when the browser view is zoomed in/out, though technically it is [more precise than eg
`clientWidth`](https://developer.mozilla.org/en-US/docs/Web/API/Element/clientWidth).

To see this for yourself, use `getBoundingClientWidth()` and `clientWidth` to measure the width of
a relatively sized element at various zoom levels. `clientWidth` reports the same value each time,
while `getBoundingClientRect()` may report slightly different values.

This very small difference may be magnified during the calculation done in `itemsPerRow()` and, in
certain cases, causes the resulting value to be 1 less than what it should be. The number of items
to render in the window are thus incorrect.

My crude understanding of the cause: when the browser calculates viewport and element
dimensions at non-100% zoom level, some native rounding is involved. `getBoundingClientRect()`,
which reports precise fractional values, is affected by this rounding, while `clientWidth` rounds or
floors values. This may not be entirely correct - discussion on this is rather sparse and anyways,
it's beyond me.

* fix(gridSystem): make `itemsPerRow` more conservative

Err on the side of rendering too many items instead of too few.
  • Loading branch information
psychedelicious authored May 2, 2023
1 parent ec97e6f commit 440f85c
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 3 deletions.
4 changes: 2 additions & 2 deletions src/gridSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,6 @@ function itemTop(viewport: ElementDimensions, gap: Gap, item: ElementDimensions,
return top > 0 ? top + gap.row : top
}

function itemsPerRow(viewportWidth: number, itemWidth: number, gap: number) {
return max(1, floor((viewportWidth + gap) / (itemWidth + gap)))
export function itemsPerRow(viewportWidth: number, itemWidth: number, gap: number) {
return max(1, floor((viewportWidth + gap) / (floor(itemWidth) + gap)))
}
56 changes: 55 additions & 1 deletion test/gridSystem.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { init, getValue, publish, subscribe } from '../src/urx'
import { gridSystem } from '../src/gridSystem'
import { gridSystem, itemsPerRow } from '../src/gridSystem'
import { describe, it, expect, vi } from 'vitest'

describe('grid system', () => {
Expand Down Expand Up @@ -183,4 +183,58 @@ describe('grid system', () => {
expect(items[0].index).toBe(0)
expect(items[items.length - 1].index).toBe(11)
})

it('correctly calculates items per row', () => {
const { itemDimensions, scrollTop, viewportDimensions, gridState, totalCount, gap } = init(gridSystem)

publish(totalCount, 2000)

publish(scrollTop, 0)

publish(gap, {
row: 5,
column: 5,
})

// Experimentally-determined values that create a rounding error

publish(viewportDimensions, {
width: 335,
height: 335,
})

publish(itemDimensions, {
width: 108.33333587646484,
height: 80,
})

expect(itemsPerRow(getValue(viewportDimensions).width, getValue(itemDimensions).width, getValue(gap).column)).toBe(3)
expect(getValue(gridState).items).toHaveLength(12)

publish(viewportDimensions, {
width: 405,
height: 505,
})

publish(itemDimensions, {
width: 131.6666717529297,
height: 80,
})

expect(itemsPerRow(getValue(viewportDimensions).width, getValue(itemDimensions).width, getValue(gap).column)).toBe(3)
expect(getValue(gridState).items).toHaveLength(18)

publish(viewportDimensions, {
width: 653,
height: 770,
})

publish(itemDimensions, {
width: 104.66667175292969,
height: 150,
})

expect(itemsPerRow(getValue(viewportDimensions).width, getValue(itemDimensions).width, getValue(gap).column)).toBe(6)
expect(getValue(gridState).items).toHaveLength(30)
})
})

0 comments on commit 440f85c

Please sign in to comment.