Fixes implicit scroll to top on android when orientation changes #187
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
naqvitalha
commented
Mar 16, 2022
naqvitalha
commented
Mar 16, 2022
| @@ -360,7 +362,15 @@ class FlashList<T> extends React.PureComponent< | |||
| ); | |||
| } | |||
|
|
|||
| private validateListSize(event: LayoutChangeEvent) { | |||
| const { height, width } = event.nativeEvent.layout; | |||
| if (Math.floor(height) <= 1 || Math.floor(width) <= 1) { | |||
There was a problem hiding this comment.
We have to give a non zero size and I've used 1. This warning will show up if lists' either dimension <2px. I need floor because onLayout can have slightly higher values in some cases.
I also like the fact that we've moved to a warning instead of an error. I think this is fair because if a list is rendered with such a small size it most like means parent uses flex-start or something similar. My intent is to only catch this issue thus I want to use smallest value possible.
We cannot skip the warning because bounded size issue happens quite a lot and showing the solution right there helps developers.
|
Please do review this and merge if everything looks good! |
fortmarek
approved these changes
Mar 16, 2022
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When orientation is changed for the first time on Android the list scrolls to the top. This does not happen on iOS. While debugging I noticed that this issue started to happen when we stopped using RLV's render footer.
RLV's footer is rendered separately from the list items so on the first render which is used to measure the scrollview this footer would give some content to scrollview which was non zero in size. I don't know the reason but if content is of zero height when scrollview mounts then on its size change it raises a scroll event with offset 0px. This looks like an issue in ScrollView and not the list.
To solve this I've to set some min size to the content. More context is present in comments with the changes.
Reviewers’ hat-rack 🎩
Video
Screen.Recording.2022-03-15.at.23.59.58.mov
Checklist