-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat(VirtualizedList)!: remove numberOfElementsVisibleOnScreen and numberOfElementsRendered props #153
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
SpatialNavigationVirtualizedList
component had a prop callednumberOfItemsVisibleOnScreen
which 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 :
numberOfItemsVisibleOnScreen
prop => Now computed automaticallynumberOfItemsRendered
propadditionalItemsRendered
with safe default valuesSwitching to the new version
In the latest version, the
numberOfItemsVisibleOnScreen
andnumberOfItemsRendered
props have been removed from the virtualized lists and grid API. ThenumberOfItemsVisibleOnScreen
is 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
numberOfItemsVisibleOnScreen
andnumberOfItemsRendered
props from all instances ofSpatialNavigationVirtualizedList
andSpatialNavigationVirtualizedGrid
.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 setadditionalItemsRendered
tonumberOfItemsRendered - 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.