feat: topItemCount for TableVirtuoso #1147
Conversation
src/TableVirtuoso.tsx
Outdated
|
|
||
| if (statefulTotalCount === 0 && EmptyPlaceholder) { | ||
| return <EmptyPlaceholder {...contextPropIfNotDomElement(EmptyPlaceholder, context)} /> | ||
| } | ||
|
|
||
| const paddingTop = listState.offsetTop + paddingTopAddition + deviation | ||
| const topItems = (showTopList ? listState.topItems : []).map((item) => { |
There was a problem hiding this comment.
This is duplicating the const items block below. I would rather have this refactored into a common routine to reduce size and avoid potential bugs because things changed in one of the blocks but were forgotten into the other.
There was a problem hiding this comment.
I made topItems and items common. Does it match your image?
78bd0c6
src/TableVirtuoso.tsx
Outdated
| const topItems = (showTopList ? listState.topItems : []).map((item) => { | ||
| const index = item.originalIndex! | ||
| const key = computeItemKey(index + firstItemIndex, item.data, context) | ||
| const offsetTop = listState.topItems.reduce((acc, item, itemIndex) => { |
There was a problem hiding this comment.
This nested loop can theoretically cause performance problems.
There was a problem hiding this comment.
Indeed. I made a list of offsets in advance and used them in the loop.
https://github.com/petyosi/react-virtuoso/pull/1147/files#diff-bbd02b2ee6e5b3189b3c2bcbed9062a411c4ad1a500e9e8fd47777a5a65d25b5R93-R100
|
Looks good, I will test and merge if everything is ok. |
|
@yuta4j1 Thank you for your contribution :). |
|
🎉 This PR is included in version 4.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Resolves #1130 .
I wanted to separate topItems from items as in
Virtuoso.tsx, but in the case ofTableVirtsuoso.tsx, the Items component contains a TableBodyComponent, so I could not put a TableRowComponent outside of it, and So, this is how it is implemented.If you have any ideas for a better implementation, I'd be happy to point them out.