Skip to content
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

[Mobile][Tech-debt] VirtualizedList inside the BottomSheet #23902

Closed
dratwas opened this issue Jul 13, 2020 · 2 comments
Closed

[Mobile][Tech-debt] VirtualizedList inside the BottomSheet #23902

dratwas opened this issue Jul 13, 2020 · 2 comments
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Performance Related to performance efforts

Comments

@dratwas
Copy link
Contributor

dratwas commented Jul 13, 2020

Our current BottomSheet wraps its content into the ScrollView component which causes a performance issue if we have VirtualizedList (FlatList) inside. We can not remove the ScrollView for all BottomSheet contents but we definitely can do that if there is FlatList or ScrollView inside. We need to wrap the content of BottomSheet into the ScrollView and use some of its props to control the swipe to close mechanism and because of that, we need to pass these props to the FlatList if we remove the scroll view. I made a fix for that here: #23873 where i pass these props by the BottomSheetContext and wrap the content into ScrollView depending of isChildrenScrollable prop. I feel like there could be a better way to handle that. I.e we could implement it similar to the KeyboardAvoidingView. We would have BottomSheetScrollView, BottomSheetFlatList, and pass all props to BottomSheet instead like here:

However, I see some kind of limitation like: We could not add something above or below the FlatList w/o additional props like Header/Footer.

I created this issue to hear your thoughts on how we could improve the BottomSheet to support VirtualizedLists inside.

cc: @pinarol @lukewalczak @mkevins @geriux

@lukewalczak
Copy link
Member

I was thinking about it and my idea was similar to adding new prop to distinguish whether content within the bottom sheet is scrollable - that solution is quite simple.

However, I see some kind of limitation like: We could not add something above or below the FlatList w/o additional props like Header/Footer.

Hmm, I think we can add props similar to FlatList: HeaderComponent and FooterComponent.

@talldan talldan added [Type] Performance Related to performance efforts Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Jul 14, 2020
@SiobhyB SiobhyB self-assigned this May 3, 2022
@SiobhyB
Copy link
Contributor

SiobhyB commented May 9, 2022

I'm going to go ahead to close this issue, as I see that #23873 was iterated on and merged. There's now a custom hasNavigation prop that determines whether content is wrapped in a ScrollView or not, as proposed in this issue.

For a longer term solution, #37559 has some options for improving stability and performance of BottomSheets. I'll make a note a note of this issue there, too.

Feel welcome to reopen if you think there's anything further that needs to be discussed/worked on or that I missed here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Performance Related to performance efforts
Projects
None yet
Development

No branches or pull requests

4 participants