PedroBern / react-native-collapsible-tab-view

A cross-platform Collapsible Tab View component for React Native
MIT License
830 stars 161 forks source link

fix: tabs should not scroll to top #205

Closed andreialecu closed 2 years ago

andreialecu commented 2 years ago

Fixes #198

github-actions[bot] commented 2 years ago

The Expo app for the example from this branch is ready!

expo.io/@pedrobern/react-native-collapsible-tab-view-demos?release-channel=pr-205

PedroBern commented 2 years ago

@andreialecu I got a bug at first glance on old devices - I don't have this bug on main.

https://user-images.githubusercontent.com/39778068/137044422-48be1c0a-d45d-4bb7-aacf-8a67aaf93429.mp4

alexco2 commented 2 years ago

@andreialecu I got a bug at first glance on old devices - I don't have this bug on main.

2021_10_12_20_56_29_trim.mp4

On a Samsung S7 (~4 years old) I was not able to recreate this bug. This was 2 days ago. Can I be of any other help?

PedroBern commented 2 years ago

Thanks @alexco2 The device I used for the screen recording is cheap and old, I like to test to check if stuff works on it ;)

But if it's good for @andreialecu , it's good for me as well 🚀

hirbod commented 2 years ago

Anything holding back to merge this?

andreialecu commented 2 years ago

I've been using this for a while in production and it seems to work fine. I'll merge it.

andreialecu commented 2 years ago

Released as 4.5.0

hirbod commented 2 years ago

@andreialecu weird, for me, it is still resetting the scroll position to top, as soon as I switch the tab.

andreialecu commented 2 years ago

Indeed there's an edge case here, it works if the tab you're moving away from is scrolled down a bit, but if one is scrolled to the top, then it will reset the others.

I'll open a subsequent PR to fix this.

hirbod commented 2 years ago

@andreialecu here is a video demonstrating, that this haven't changed anything: https://streamable.com/wr7mrh

As soon as I change the tab, my previous view will scroll to top, which is pretty annoying, because I would love to maintain the scroll position (which is also pretty important for FlatList performance, as it currently retriggers all the VirzualizedList events)

hirbod commented 2 years ago

For me, it only works, when every lazy loaded tab was loaded and I scrolled within every single one of them once. Please also consider that tabs can be unrendered yet.

andreialecu commented 2 years ago

@Hirbod https://github.com/PedroBern/react-native-collapsible-tab-view/pull/230 should fix it, it was just released as 4.5.2

hirbod commented 2 years ago

@andreialecu it's better but still not 100% fixed.

I have 4 tabs (with prop lazy). As soon as the lazy one loads, it syncs all the other screens top position. See following video (i have added indexes to the views):

So there is still an edge case to be handled I guess. https://streamable.com/vk9n8t

1 and 2 working fine. When 3 comes it, it syncs tabs 1,2,3

When 4 comes in, it syncs tabs 1,2,3,4

andreialecu commented 2 years ago

I'm not able to reproduce this. See this recording from the example app:

https://user-images.githubusercontent.com/697707/149623449-172d9b8a-783b-4de5-bc2c-3b6c0f73aff4.mov

hirbod commented 2 years ago

Sorry, here is the updated video. https://streamable.com/vk9n8t

You can cleary see, the second tab 3 got rendered, tab 1 and tab 2 had the same indexes at top (like the third one coming in). Same happens when the 4th tab renderes. After that, the sync works fine.

andreialecu commented 2 years ago

Oh, ok, I see now.

hirbod commented 2 years ago

@andreialecu And there is another edge case. It will restore all scroll positions to top as soon as I fully scroll up (uncollapsing the header) https://streamable.com/b1bug4

Would be wonderful if you could fix those last two remaining little bugs, then this lib is perfect :D

hirbod commented 2 years ago

@andreialecu did you had a chance to have a last look at the issue? Putting $50 bounty on it!

hirbod commented 2 years ago

@andreialecu any chance?

andreialecu commented 2 years ago

@andreialecu And there is another edge case. It will restore all scroll positions to top as soon as I fully scroll up (uncollapsing the header) streamable.com/b1bug4

Would be wonderful if you could fix those last two remaining little bugs, then this lib is perfect :D

If one tab is at the top, with the header revealed, then if you switch to a different tab then the header is still revealed, and it only makes sense to be scrolled to the top in that tab as well.

If that didn't happen, the flatlist would look glitched. I'm not entirely sure what you'd expect to happen here, care to elaborate?

hirbod commented 2 years ago

@andreialecu I agree with the last edge case, more likely I am still stumbling across this bug:

Sorry, here is the updated video. https://streamable.com/vk9n8t

You can cleary see, the second tab 3 got rendered, tab 1 and tab 2 had the same indexes at top (like the third one coming in). Same happens when the 4th tab renderes. After that, the sync works fine.

andreialecu commented 2 years ago

Are you able to reproduce this in the example app that comes with this repository? If you can, can you share a video that shows it on the example app?

hirbod commented 2 years ago

I'll need to set it up. Will get back to you after its running on my device