Updated VirtualizedList vendor files (with FlatList, SectionList) #2241
Conversation
…edUtils from fbrn latest. Manually maintained the imports lists due to different syntaxes and such. Skipped one new feature but tracked that task with a TODO marker.
…edList from fbrn source. Took some minor function renames that were skipped before, and aligned import order to match for easier future merge maintenance.
| @@ -733,19 +733,6 @@ class VirtualizedList extends React.PureComponent<Props, State> { | |||
| } | |||
| } | |||
|
|
|||
| // For issue https://github.com/necolas/react-native-web/issues/995 | |||
| this.invertedWheelEventHandler = (ev: any) => { | |||
There was a problem hiding this comment.
This reverts a recent patch. What is the plan for handling inverted lists?
There was a problem hiding this comment.
Yikes, it looks like it would be easy to pull back in, so I'll give that a shot soon. I'll add louder comments to it as well to help preserve in future merges.
There was a problem hiding this comment.
Alternatively we try to fix this properly using the approach here #2233 (comment)
There was a problem hiding this comment.
Ok, although I don't understand the utility of the inverted option or inverting the scroll wheel direction, I have local changes which I can see through localhost:3000/lists does look successfully restore the behavior introduced by #2223. Here is a PR and commit which does so: DavidRieman#1.
Unfortunately something weird happened to git's interpretation of the relationship between our repositories. Github offered to merge necolas/react-native-web into davidrieman/react-native-web and I hesitated because I'd seen my PRs "Closed" (yet somehow merged) rather than "Merged" in GitHub - but ultimately I took it up on that auto-merge. After that, however, I was unable to any git operations including git pull anymore without crazy blocking errors (and online was no help) and had to re-clone my own repository locally to get back up and running. Now I've committed these changes, but GitHub claims that the diff from my branch to your master is massive. In these sorts of instances I find it safest to have a branch on the target repository cherry-pick the minimal commit to avoid whatever problems crept into place from the merge flow. Would you want to cherry-pick 33e4290 to your repo, or repeat the changes here? Or should we handle another way?
There was a problem hiding this comment.
Oh wow, I thought all my PRs here were merged and in 17.7. Just not this one. Yeah I see now git will let me recover this branch and commit the fix. So sorry for my misunderstanding, I'll try to have this patched up today.
There was a problem hiding this comment.
And cherry-pick for that commit to this branch worked. Ready for PR review. 🎉
There was a problem hiding this comment.
I need an answer to this comment please #2241 (comment)
There was a problem hiding this comment.
Which issues are resolved?
Yes: The part of 1608 where I linked to my SO, and #2030
Yes partially (when combined with another upcoming potential patch, the behavior is improved, though still not perfect): #1798, #2026
Probably Not: #1292, #1579, #1608, #1873, #1880
No: #1254, #1854, #2233, #2239 (this one was filed as work that should follow later, but involves upgrading ScrollView itself which I didn't want to touch)
N/A: #2150 (still needs a full repro scenario IMO), #2167 (different issue, replied)
…eel scroll position while list is inverted. Added loud comments to make it clear this code intends to live on in RNW even though not present in RN.
|
This patch is part of the 0.18 preview #2248 |
Per discussion at #2236, this is a direct upgrade of a handful of select
react-native-web/src/vendor/react-nativecomponents to reflect all the accumulated bug fixes and improvements made to theirreact-nativeroots since they were last updated some time last year. I have tested these changes with some newyarn examplespages which are up for review separately. (Most importantly, temporarily upgrading those samples to have hundreds of items used to produce major virtualization bugs which we can now see are solved with the latest versions of these files.) Theyarn testsalso pass the same tests as before.The upgrade process was simply done by 'diffing' the latest code from the react-native repo against the mainline here. (Fortunately, I found WebStorm has a handy "Compare with Clipboard" feature which puts it into merge resolution mode.) At first I was selective, but in the end with my final passes, I accepted the vast majority of changes from react-native for these, and even put the
importorder here into parity to make further diffs (and consequently, taking future bug fixes from react-native) much easier going forward.In a couple small cases, I had to make up a type since we don't have the equivalent classes. The import order parity should help identify any cases like that as well, as they'll be a type declaration where react-native used a type import. Where I could, I tried to follow existing precedent for similar types in other react-native-web files.
There is one dev-only feature which was new to VirtualizedList, which I intentionally avoided trying to add for now. Trying to add the feature would simply add too much scope to a single PR. Instead, I logged #2239 and cited that ticket at the disabled code. I left the code just commented out there in order to call out the missing feature, and also to reduce scope of any future diffs that may happen to occur before that ticket can be tackled properly.