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

[Nicer Tabs] New native pager #6868

Merged
merged 30 commits into from
Dec 3, 2024
Merged

[Nicer Tabs] New native pager #6868

merged 30 commits into from
Dec 3, 2024

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Nov 30, 2024

This revives #1696 but in a performant way.

Test Plan

Try different interactions on iOS and Android:

  • Initial load: The pager should autoscroll to the initially selected tab on reload. (Ideally this would not be animated, but since I couldn't figure out a reliable way to do it on the first frame on both platforms, I have to animate it.)
  • Swiping the pager drives tabbar scroll: The swiping gesture should stay in sync with the tabbar scroll. A full page swipe should bring you to the same position as tapping the next item. Note that if you continuously swipe page after page, by the time you arrive at the last page, the blue line should naturally be at the right edge of the screen. This also means that by the time you're halfway through the feeds, the blue line should be exactly centered in the middle. This is easy to see if you have an odd number of feeds.
  • Tapping predictably aligns the tabbar: Tapping to select an item will adjust the tabbar to its "synced" position, same as a swipe. There is one exception to this, which I'll cover next.
  • Scrolling the tabbar itself temporarily unsyncs it: If you start scrolling the tabbar and then tap on an item there, if that item is fully visible in view (with a margin of 20 on each side), that tap will not change the tabbar scroll position. This is to avoid things shifting from "right under you" on tap. However, the next tap will be synced as usual (unless you scroll again). If the item was not fully visible into view (with a margin of 20 on each side), we always sync.
  • Unsyncing, then swiping: You can unsync the tabbar (by scrolling it) and then swipe. In this case, we won't mess with the tabbar scroll position during the swipe — but it'll get re-sync immediately after you release the gesture.
  • Unsyncing, then tapping, then swiping: Unsync the tabbar (by scrolling it as far as you want). Then tap on some item that's fully in view. The item should get selected but the tabbar shouldn't scroll "under" you. Now start swiping. Notice that the gesture will not be jumpy — it'll gradually move as always but then resync after you release.

Verify non-primary cases:

  • Just one tab: Check a feed's page
  • Just two tabs: Delete feeds other than Following and Discover
  • The profile: Verify profile tabs behave as expected
  • Web: Should have no changes

Verify perf on Android and iOS devices.

Known issues:

  • Due to the change in [Nicer Tabs] Fork TabBar, simplify Pager #6762, there's no prewarming during swipe, so each feed loads a bit "too late" (after swipe). I'll stack something on top of this PR to resolve that, but not in scope.

    • Actually, this doesn't seem very relevant — we were only starting load when we got into a "settling" state so it was pretty late anyway. I'll look at improving this regardless but doesn't block land.
  • On Android, swipes are sometimes interpreted as taps and bring up the lightbox or navigate into the post. This is not a new issue, and has always been broken — I'll likely look at it but not today.

pager.mov

Copy link

github-actions bot commented Nov 30, 2024

Old size New size Diff
8.14 MB 8.14 MB 0 B (0.00%)

@arcalinea arcalinea temporarily deployed to nicer-tabs-2 - social-app PR #6868 November 30, 2024 19:00 — with Render Destroyed
@gaearon gaearon changed the base branch from main to nicer-tabs-1 November 30, 2024 19:01
@arcalinea arcalinea temporarily deployed to nicer-tabs-2 - social-app PR #6868 November 30, 2024 19:50 — with Render Destroyed
@gaearon gaearon marked this pull request as ready for review November 30, 2024 20:20
@gaearon gaearon mentioned this pull request Nov 30, 2024
@arcalinea arcalinea temporarily deployed to nicer-tabs-2 - social-app PR #6868 December 2, 2024 14:49 — with Render Destroyed
@gaearon gaearon requested review from mozzius, estrattonbailey, pfrazee and haileyok and removed request for mozzius and estrattonbailey December 2, 2024 16:07
Comment on lines +154 to +155
dragProgress={undefined as any /* native-only */}
dragState={undefined as any /* native-only */}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a way to express platforms in the type system :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably hack something together for this in the future

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, some sort of monad perhaps

Base automatically changed from nicer-tabs-1 to main December 3, 2024 01:13
@arcalinea arcalinea temporarily deployed to nicer-tabs-2 - social-app PR #6868 December 3, 2024 01:14 — with Render Destroyed
Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested for a full day and worked fine. Code lgtm

@gaearon gaearon merged commit cd81111 into main Dec 3, 2024
6 checks passed
@gaearon gaearon deleted the nicer-tabs-2 branch December 3, 2024 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants