fix(gridSystem.ts): call onEndReached when the data is less than cont… #1166
Conversation
src/gridSystem.ts
Outdated
| @@ -253,17 +253,21 @@ export const gridSystem = /*#__PURE__*/ u.system( | |||
| } else { | |||
| startIndex = perRow * floor((startOffset + rowGap) / (itemHeight + rowGap)) | |||
| endIndex = perRow * ceil((endOffset + rowGap) / (itemHeight + rowGap)) - 1 | |||
| endIndex = min(totalCount - 1, max(endIndex, perRow - 1)) | |||
| endIndex = min((data?.length || totalCount || 0) - 1, max(endIndex, perRow - 1)) | |||
There was a problem hiding this comment.
I'll prepare a video later today to explain but here's the gist:
When both data and totalCount is passed to VirtuosoGrid - the endIndex calculation was faulty. Now looking at this after a good night sleep, it seems like we might not require this as we update totalCount below. I'll get back on this
There was a problem hiding this comment.
Okay here's the explanation why this change is necessary:
In a component with paginated data (data received from useInfiniteQuery), onEndReached function fires the callback to load more data - that eventually increases the data size. Additionally there might be a different totalIndex specified to calculate the container height. So usually totalCount could be greater than or equal to data.length
Hence the endIndex should consider the min of data.length or totalCount to get the right value
src/gridSystem.ts
Outdated
| u.combineLatest(data, totalCount), | ||
| u.filter(([data]) => data !== null), | ||
| u.map(([data, totalCount]) => { | ||
| return Math.max(data!.length, totalCount) | ||
| }), | ||
| u.distinctUntilChanged() |
There was a problem hiding this comment.
Why are you changing this? The component should accept either data or total count, and if data is supplied, it should control the total count value throughout the rest of the implementation.
There was a problem hiding this comment.
This is necessary to calculate the right totalCount when both data and totalCount is passed to VirtuosoGrid:
<VirtuosoGrid
data={arrayOf20Items}
totalCount={100}
/>
There was a problem hiding this comment.
Having both data and total count is not a valid configuration and will result in a weird inconsistent implementation.
There was a problem hiding this comment.
But it is a common use case for something like a dynamic list (or infinite list)
There was a problem hiding this comment.
I can see how one can potentially achieve some elaborate implementation using a combination of both props and load on demand, and conditional rendering depending on data being present or not.
However, this is way beyond trivial, and supporting this would need a good set of examples and tests. If these are not present, the capability would just result in people struggling infinitely and claiming "bugs" that I need to deal with.
There was a problem hiding this comment.
I see.. Gotcha, maybe I touched a bit more code than necessary. I'll make some changes
There was a problem hiding this comment.
@petyosi take a look now, I have added an example page and updated code to only touch endReached fn.
src/gridSystem.ts
Outdated
| // const rowCount = ceil(totalCount / perRow) // this is the total number of rows based on `totalCount` | ||
| const rowsVisibleCount = ceil((data?.length || totalCount || 0) / perRow) // number of rows rendered on screen | ||
|
|
||
| return { items, offsetTop: top, offsetBottom, top, bottom, itemHeight, itemWidth } as GridState | ||
| // const totalHeight = rowCount * itemHeight + (rowCount - 1) * rowGap // this is the total height that the container should occupy | ||
| const virtualHeight = rowsVisibleCount * itemHeight + (rowsVisibleCount - 1) * rowGap // height visible | ||
|
|
||
| const offsetBottom = virtualHeight - bottom | ||
|
|
||
| return { items, offsetTop: top, offsetBottom, top, bottom, itemHeight, itemWidth, data } as GridState |
There was a problem hiding this comment.
Unless I am missing something, this change should not do anything. See my comment on the relationship between totalCount and data.
There was a problem hiding this comment.
My bad, I added data here for testing, forgot removing it. Will update this later today
src/gridSystem.ts
Outdated
| ) | ||
| }), | ||
| u.map(([[_, data]]) => { | ||
| return data?.length |
There was a problem hiding this comment.
How does that work if the grid has no data and uses totalCount?
There was a problem hiding this comment.
Good question. It does work but now I'm thinking maybe I don't need this. This change was made before updating the totalCount logic, but I'll revisit. Maybe I was way too sleepy while raising this PR
a8611ff to
4422e3a
Compare
src/gridSystem.ts
Outdated
| u.withLatestFrom(totalCount, hasScrolled, data), | ||
| u.filter(([[gridState, data], _totalCount, hasScrolled]) => { | ||
| const lastIndex = gridState.items[gridState.items.length - 1].index | ||
| const isLastItemRendered = lastIndex === (data?.length || 0) - 1 |
There was a problem hiding this comment.
Why data length and not totalCount?
src/gridSystem.ts
Outdated
|
|
||
| // User has not scrolled, so check whether grid is fully rendered | ||
| const isFullyRendered = | ||
| gridState.bottom > 0 && gridState.itemHeight > 0 && gridState.offsetBottom === 0 && gridState.items.length === data?.length |
There was a problem hiding this comment.
Why data length and not totalCount?
There was a problem hiding this comment.
It's strange but using totalCount here doesn't fire more than once. But I think I found a way to get around it. Hold on, sending an update
There was a problem hiding this comment.
@petyosi check now :)
|
🎉 This PR is included in version 4.12.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR fixes this bug: #1087