-
Notifications
You must be signed in to change notification settings - Fork 42
feat(VirtualizedList)!: remove numberOfElementsVisibleOnScreen and numberOfElementsRendered props #153
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
Conversation
...ges/lib/src/spatial-navigation/components/virtualizedList/getNumberOfItemsVisibleOnScreen.ts
Outdated
Show resolved
Hide resolved
.../spatial-navigation/components/virtualizedList/helpers/getAdditionalNumberOfItemsRendered.ts
Show resolved
Hide resolved
2f518fb to
80276ad
Compare
...s/lib/src/spatial-navigation/components/virtualizedGrid/SpatialNavigationVirtualizedGrid.tsx
Show resolved
Hide resolved
80276ad to
5ffeda3
Compare
| return minSize; | ||
| }; | ||
| export const getNumberOfItemsVisibleOnScreen = <T>( | ||
| data: T[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
déso je débarque un peu car j'ai quasiment jamais relu de PR sur la lib 😬
Mais data c'est pas très clair. C'est des items ? Autant que le naming soit cohérent vu qu'on a itemSize juste derriere.
Le type générique est pas clair non plus: T ça veut rien dire -> c'est ItemType ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(je réponds en anglais comme c'est public)
We used "data" to be as close as possible to the FlatList API. That allows us in the future to have an easier time when working with universal apps :) (FlatList on mobile, SpatialNavigationVirtualizedList on TV, and props will be similar)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against using data as the API prop. But I don't think we should use it internally. We can always add an adaptation layer, or at least use item everywhere else than inside the exported component
I'm also unresolving the comment because I don't think the type should be called T 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this is not resolved!
I totally agree with T that could be named TData for consistency. But I think I disagree with data renaming: it's quite obvious that we're talking about the data props, untransformed, that's given to the list. I think renaming it internally would create add more confusion than clarity 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still thinking about it: I think it really makes sense to rename data at some point internally, but we need to think the name through and do it consistently everywhere 😁
It could be the purpose of a refactoring PR!
...src/spatial-navigation/components/virtualizedList/helpers/getNumberOfItemsVisibleOnScreen.ts
Outdated
Show resolved
Hide resolved
...src/spatial-navigation/components/virtualizedList/helpers/getNumberOfItemsVisibleOnScreen.ts
Outdated
Show resolved
Hide resolved
1abc64a to
dab9e29
Compare
...src/spatial-navigation/components/virtualizedList/helpers/getNumberOfItemsVisibleOnScreen.ts
Outdated
Show resolved
Hide resolved
...src/spatial-navigation/components/virtualizedList/helpers/getNumberOfItemsVisibleOnScreen.ts
Show resolved
Hide resolved
...src/spatial-navigation/components/virtualizedList/helpers/getNumberOfItemsVisibleOnScreen.ts
Show resolved
Hide resolved
a50198f to
28235ed
Compare
Previously, the
SpatialNavigationVirtualizedListcomponent had a prop callednumberOfItemsVisibleOnScreenwhich has been renamed toadditionalItemsRendered. This simplifies the component API and provides the end user an easy way to setup a virtualized list with safe rendering and virtualization.Same change was made to 'SpatialNavigationVirtualizedGrid`.
Summary :
numberOfItemsVisibleOnScreenprop => Now computed automaticallynumberOfItemsRenderedpropadditionalItemsRenderedwith safe default valuesSwitching to the new version
In the latest version, the
numberOfItemsVisibleOnScreenandnumberOfItemsRenderedprops have been removed from the virtualized lists and grid API. ThenumberOfItemsVisibleOnScreenis now calculated automatically based on the parent view size.Both of these props have been replaced by a new optional prop called
additionalItemsRendered. This prop controls the number of items rendered just outside the visible screen (but not yet virtualized). It allows you to fine-tune the virtualization size without the need to manually calculatenumberOfItemsVisibleOnScreen, which could lead to incorrect values.How to Update to the New Version
To migrate to the new version, you'll need to remove the
numberOfItemsVisibleOnScreenandnumberOfItemsRenderedprops from all instances ofSpatialNavigationVirtualizedListandSpatialNavigationVirtualizedGrid.By default, you don't need to provide a value for
additionalItemsRendered. However, if you want to replicate the behavior of the previous version exactly, you can setadditionalItemsRenderedtonumberOfItemsRendered - numberOfItemsVisibleOnScreen. This will ensure the same number of off-screen items are rendered.Impact on Tests
Your tests may also break, as the number of visible items is now calculated based on the size of the parent view. This number will vary depending on the execution environment. For example, in Jest, where the window width is 750 px, the calculation will be based on that width.