From 5a313c2d10b112458830b3bfc708031f6f8726a0 Mon Sep 17 00:00:00 2001 From: dan Date: Tue, 3 Dec 2024 01:12:58 +0000 Subject: [PATCH] [Nicer Tabs] Fork TabBar, simplify Pager (#6762) * Fork TabBar.web.tsx * Trim dead code from both forks * Remove onPageSelecting event It's difficult to tell what exactly it's supposed to represent, and in practice it's not really used aside from logging. Let's rip it out for now to keep other changes simpler. * Remove early onPageSelected call It was added to try to do some work eagerly when we're sure which way the scroll is snapping. This is not necessarily a good idea though. It schedules a potentially expensive re-render right during the deceleration animation, which is not great. Whatever we're optimizing there, we should optimize smarter (e.g. prewarm just the network call). The other thing it used to help with is triggering the pager header autoscroll earlier. But we're going to rewrite that part differently anyway so that's not relevant either. * Prune more dead code from the native version We'll have to revisit this when adding tablet support but for now I'd prefer to remove a codepath that is not being tested or ever run. * Use regular ScrollView on native The Draggable thing was needed for web-only behavior so we can drop it in the native fork. --- src/lib/statsig/events.ts | 6 - src/view/com/pager/Pager.tsx | 70 +------- src/view/com/pager/Pager.web.tsx | 20 +-- src/view/com/pager/PagerWithHeader.tsx | 5 - src/view/com/pager/PagerWithHeader.web.tsx | 5 - src/view/com/pager/TabBar.tsx | 102 ++--------- src/view/com/pager/TabBar.web.tsx | 192 +++++++++++++++++++++ src/view/screens/Home.tsx | 15 +- 8 files changed, 216 insertions(+), 199 deletions(-) create mode 100644 src/view/com/pager/TabBar.web.tsx diff --git a/src/lib/statsig/events.ts b/src/lib/statsig/events.ts index f8c6d181c47..674562f8241 100644 --- a/src/lib/statsig/events.ts +++ b/src/lib/statsig/events.ts @@ -80,12 +80,6 @@ export type LogEvents = { feedUrl: string feedType: string index: number - reason: - | 'focus' - | 'tabbar-click' - | 'pager-swipe' - | 'desktop-sidebar-click' - | 'starter-pack-initial-feed' } 'feed:endReached': { feedUrl: string diff --git a/src/view/com/pager/Pager.tsx b/src/view/com/pager/Pager.tsx index de040999172..f0e686b6abc 100644 --- a/src/view/com/pager/Pager.tsx +++ b/src/view/com/pager/Pager.tsx @@ -1,21 +1,16 @@ import React, {forwardRef} from 'react' import {View} from 'react-native' import PagerView, { - PagerViewOnPageScrollEvent, PagerViewOnPageSelectedEvent, PageScrollStateChangedNativeEvent, } from 'react-native-pager-view' -import {LogEvents} from '#/lib/statsig/events' import {atoms as a, native} from '#/alf' export type PageSelectedEvent = PagerViewOnPageSelectedEvent export interface PagerRef { - setPage: ( - index: number, - reason: LogEvents['home:feedDisplayed']['reason'], - ) => void + setPage: (index: number) => void } export interface RenderTabBarFnProps { @@ -29,10 +24,6 @@ interface Props { initialPage?: number renderTabBar: RenderTabBarFn onPageSelected?: (index: number) => void - onPageSelecting?: ( - index: number, - reason: LogEvents['home:feedDisplayed']['reason'], - ) => void onPageScrollStateChanged?: ( scrollState: 'idle' | 'dragging' | 'settling', ) => void @@ -46,24 +37,16 @@ export const Pager = forwardRef>( renderTabBar, onPageScrollStateChanged, onPageSelected, - onPageSelecting, testID, }: React.PropsWithChildren, ref, ) { const [selectedPage, setSelectedPage] = React.useState(0) - const lastOffset = React.useRef(0) - const lastDirection = React.useRef(0) - const scrollState = React.useRef('') const pagerView = React.useRef(null) React.useImperativeHandle(ref, () => ({ - setPage: ( - index: number, - reason: LogEvents['home:feedDisplayed']['reason'], - ) => { + setPage: (index: number) => { pagerView.current?.setPage(index) - onPageSelecting?.(index, reason) }, })) @@ -75,60 +58,18 @@ export const Pager = forwardRef>( [setSelectedPage, onPageSelected], ) - const onPageScroll = React.useCallback( - (e: PagerViewOnPageScrollEvent) => { - const {position, offset} = e.nativeEvent - if (offset === 0) { - // offset hits 0 in some awkward spots so we ignore it - return - } - // NOTE - // we want to call `onPageSelecting` as soon as the scroll-gesture - // enters the "settling" phase, which means the user has released it - // we can't infer directionality from the scroll information, so we - // track the offset changes. if the offset delta is consistent with - // the existing direction during the settling phase, we can say for - // certain where it's going and can fire - // -prf - if (scrollState.current === 'settling') { - if (lastDirection.current === -1 && offset < lastOffset.current) { - onPageSelecting?.(position, 'pager-swipe') - setSelectedPage(position) - lastDirection.current = 0 - } else if ( - lastDirection.current === 1 && - offset > lastOffset.current - ) { - onPageSelecting?.(position + 1, 'pager-swipe') - setSelectedPage(position + 1) - lastDirection.current = 0 - } - } else { - if (offset < lastOffset.current) { - lastDirection.current = -1 - } else if (offset > lastOffset.current) { - lastDirection.current = 1 - } - } - lastOffset.current = offset - }, - [lastOffset, lastDirection, onPageSelecting], - ) - const handlePageScrollStateChanged = React.useCallback( (e: PageScrollStateChangedNativeEvent) => { - scrollState.current = e.nativeEvent.pageScrollState onPageScrollStateChanged?.(e.nativeEvent.pageScrollState) }, - [scrollState, onPageScrollStateChanged], + [onPageScrollStateChanged], ) const onTabBarSelect = React.useCallback( (index: number) => { pagerView.current?.setPage(index) - onPageSelecting?.(index, 'tabbar-click') }, - [pagerView, onPageSelecting], + [pagerView], ) return ( @@ -142,8 +83,7 @@ export const Pager = forwardRef>( style={[a.flex_1]} initialPage={initialPage} onPageScrollStateChanged={handlePageScrollStateChanged} - onPageSelected={onPageSelectedInner} - onPageScroll={onPageScroll}> + onPageSelected={onPageSelectedInner}> {children} diff --git a/src/view/com/pager/Pager.web.tsx b/src/view/com/pager/Pager.web.tsx index e6909fe10f7..c620e73e33f 100644 --- a/src/view/com/pager/Pager.web.tsx +++ b/src/view/com/pager/Pager.web.tsx @@ -2,7 +2,6 @@ import React from 'react' import {View} from 'react-native' import {flushSync} from 'react-dom' -import {LogEvents} from '#/lib/statsig/events' import {s} from '#/lib/styles' export interface RenderTabBarFnProps { @@ -16,10 +15,6 @@ interface Props { initialPage?: number renderTabBar: RenderTabBarFn onPageSelected?: (index: number) => void - onPageSelecting?: ( - index: number, - reason: LogEvents['home:feedDisplayed']['reason'], - ) => void } export const Pager = React.forwardRef(function PagerImpl( { @@ -27,7 +22,6 @@ export const Pager = React.forwardRef(function PagerImpl( initialPage = 0, renderTabBar, onPageSelected, - onPageSelecting, }: React.PropsWithChildren, ref, ) { @@ -36,16 +30,13 @@ export const Pager = React.forwardRef(function PagerImpl( const anchorRef = React.useRef(null) React.useImperativeHandle(ref, () => ({ - setPage: ( - index: number, - reason: LogEvents['home:feedDisplayed']['reason'], - ) => { - onTabBarSelect(index, reason) + setPage: (index: number) => { + onTabBarSelect(index) }, })) const onTabBarSelect = React.useCallback( - (index: number, reason: LogEvents['home:feedDisplayed']['reason']) => { + (index: number) => { const scrollY = window.scrollY // We want to determine if the tabbar is already "sticking" at the top (in which // case we should preserve and restore scroll), or if it is somewhere below in the @@ -64,7 +55,6 @@ export const Pager = React.forwardRef(function PagerImpl( flushSync(() => { setSelectedPage(index) onPageSelected?.(index) - onPageSelecting?.(index, reason) }) if (isSticking) { const restoredScrollY = scrollYs.current[index] @@ -75,7 +65,7 @@ export const Pager = React.forwardRef(function PagerImpl( } } }, - [selectedPage, setSelectedPage, onPageSelected, onPageSelecting], + [selectedPage, setSelectedPage, onPageSelected], ) return ( @@ -83,7 +73,7 @@ export const Pager = React.forwardRef(function PagerImpl( {renderTabBar({ selectedPage, tabBarAnchor: , - onSelect: e => onTabBarSelect(e, 'tabbar-click'), + onSelect: e => onTabBarSelect(e), })} {React.Children.map(children, (child, i) => ( diff --git a/src/view/com/pager/PagerWithHeader.tsx b/src/view/com/pager/PagerWithHeader.tsx index 92b98dc2e6a..1aa45ffba7e 100644 --- a/src/view/com/pager/PagerWithHeader.tsx +++ b/src/view/com/pager/PagerWithHeader.tsx @@ -182,17 +182,12 @@ export const PagerWithHeader = React.forwardRef( [onPageSelected, setCurrentPage], ) - const onPageSelecting = React.useCallback((index: number) => { - setCurrentPage(index) - }, []) - return ( {toArray(children) .filter(Boolean) diff --git a/src/view/com/pager/PagerWithHeader.web.tsx b/src/view/com/pager/PagerWithHeader.web.tsx index e72c1f3cc93..dd00264050c 100644 --- a/src/view/com/pager/PagerWithHeader.web.tsx +++ b/src/view/com/pager/PagerWithHeader.web.tsx @@ -75,17 +75,12 @@ export const PagerWithHeader = React.forwardRef( [onPageSelected, setCurrentPage], ) - const onPageSelecting = React.useCallback((index: number) => { - setCurrentPage(index) - }, []) - return ( {toArray(children) .filter(Boolean) diff --git a/src/view/com/pager/TabBar.tsx b/src/view/com/pager/TabBar.tsx index 4e8646c605e..3f453971c85 100644 --- a/src/view/com/pager/TabBar.tsx +++ b/src/view/com/pager/TabBar.tsx @@ -2,11 +2,8 @@ import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react' import {LayoutChangeEvent, ScrollView, StyleSheet, View} from 'react-native' import {usePalette} from '#/lib/hooks/usePalette' -import {useWebMediaQueries} from '#/lib/hooks/useWebMediaQueries' -import {isNative} from '#/platform/detection' import {PressableWithHover} from '../util/PressableWithHover' import {Text} from '../util/text/Text' -import {DraggableScrollView} from './DraggableScrollView' export interface TabBarProps { testID?: string @@ -31,68 +28,20 @@ export function TabBar({ }: TabBarProps) { const pal = usePalette('default') const scrollElRef = useRef(null) - const itemRefs = useRef>([]) const [itemXs, setItemXs] = useState([]) const indicatorStyle = useMemo( () => ({borderBottomColor: indicatorColor || pal.colors.link}), [indicatorColor, pal], ) - const {isDesktop, isTablet} = useWebMediaQueries() - const styles = isDesktop || isTablet ? desktopStyles : mobileStyles useEffect(() => { - if (isNative) { - // On native, the primary interaction is swiping. - // We adjust the scroll little by little on every tab change. - // Scroll into view but keep the end of the previous item visible. - let x = itemXs[selectedPage] || 0 - x = Math.max(0, x - OFFSCREEN_ITEM_WIDTH) - scrollElRef.current?.scrollTo({x}) - } else { - // On the web, the primary interaction is tapping. - // Scrolling under tap feels disorienting so only adjust the scroll offset - // when tapping on an item out of view--and we adjust by almost an entire page. - const parent = scrollElRef?.current?.getScrollableNode?.() - if (!parent) { - return - } - const parentRect = parent.getBoundingClientRect() - if (!parentRect) { - return - } - const { - left: parentLeft, - right: parentRight, - width: parentWidth, - } = parentRect - const child = itemRefs.current[selectedPage] - if (!child) { - return - } - const childRect = child.getBoundingClientRect?.() - if (!childRect) { - return - } - const {left: childLeft, right: childRight, width: childWidth} = childRect - let dx = 0 - if (childRight >= parentRight) { - dx += childRight - parentRight - dx += parentWidth - childWidth - OFFSCREEN_ITEM_WIDTH - } else if (childLeft <= parentLeft) { - dx -= parentLeft - childLeft - dx -= parentWidth - childWidth - OFFSCREEN_ITEM_WIDTH - } - let x = parent.scrollLeft + dx - x = Math.max(0, x) - x = Math.min(x, parent.scrollWidth - parentWidth) - if (dx !== 0) { - parent.scroll({ - left: x, - behavior: 'smooth', - }) - } - } - }, [scrollElRef, itemXs, selectedPage, styles]) + // On native, the primary interaction is swiping. + // We adjust the scroll little by little on every tab change. + // Scroll into view but keep the end of the previous item visible. + let x = itemXs[selectedPage] || 0 + x = Math.max(0, x - OFFSCREEN_ITEM_WIDTH) + scrollElRef.current?.scrollTo({x}) + }, [scrollElRef, itemXs, selectedPage]) const onPressItem = useCallback( (index: number) => { @@ -122,7 +71,7 @@ export function TabBar({ testID={testID} style={[pal.view, styles.outer]} accessibilityRole="tablist"> - (itemRefs.current[i] = node as any)} onLayout={e => onItemLayout(e, i)} style={styles.item} hoverStyle={pal.viewLight} @@ -143,7 +91,7 @@ export function TabBar({ ) })} - + ) } -const desktopStyles = StyleSheet.create({ - outer: { - flexDirection: 'row', - width: 598, - }, - contentContainer: { - paddingHorizontal: 0, - backgroundColor: 'transparent', - }, - item: { - paddingTop: 14, - paddingHorizontal: 14, - justifyContent: 'center', - }, - itemInner: { - paddingBottom: 12, - borderBottomWidth: 3, - borderBottomColor: 'transparent', - }, - outerBottomBorder: { - position: 'absolute', - left: 0, - right: 0, - top: '100%', - borderBottomWidth: StyleSheet.hairlineWidth, - }, -}) - -const mobileStyles = StyleSheet.create({ +const styles = StyleSheet.create({ outer: { flexDirection: 'row', }, diff --git a/src/view/com/pager/TabBar.web.tsx b/src/view/com/pager/TabBar.web.tsx new file mode 100644 index 00000000000..4291a053b50 --- /dev/null +++ b/src/view/com/pager/TabBar.web.tsx @@ -0,0 +1,192 @@ +import {useCallback, useEffect, useMemo, useRef} from 'react' +import {ScrollView, StyleSheet, View} from 'react-native' + +import {usePalette} from '#/lib/hooks/usePalette' +import {useWebMediaQueries} from '#/lib/hooks/useWebMediaQueries' +import {PressableWithHover} from '../util/PressableWithHover' +import {Text} from '../util/text/Text' +import {DraggableScrollView} from './DraggableScrollView' + +export interface TabBarProps { + testID?: string + selectedPage: number + items: string[] + indicatorColor?: string + onSelect?: (index: number) => void + onPressSelected?: (index: number) => void +} + +// How much of the previous/next item we're showing +// to give the user a hint there's more to scroll. +const OFFSCREEN_ITEM_WIDTH = 20 + +export function TabBar({ + testID, + selectedPage, + items, + indicatorColor, + onSelect, + onPressSelected, +}: TabBarProps) { + const pal = usePalette('default') + const scrollElRef = useRef(null) + const itemRefs = useRef>([]) + const indicatorStyle = useMemo( + () => ({borderBottomColor: indicatorColor || pal.colors.link}), + [indicatorColor, pal], + ) + const {isDesktop, isTablet} = useWebMediaQueries() + const styles = isDesktop || isTablet ? desktopStyles : mobileStyles + + useEffect(() => { + // On the web, the primary interaction is tapping. + // Scrolling under tap feels disorienting so only adjust the scroll offset + // when tapping on an item out of view--and we adjust by almost an entire page. + const parent = scrollElRef?.current?.getScrollableNode?.() + if (!parent) { + return + } + const parentRect = parent.getBoundingClientRect() + if (!parentRect) { + return + } + const { + left: parentLeft, + right: parentRight, + width: parentWidth, + } = parentRect + const child = itemRefs.current[selectedPage] + if (!child) { + return + } + const childRect = child.getBoundingClientRect?.() + if (!childRect) { + return + } + const {left: childLeft, right: childRight, width: childWidth} = childRect + let dx = 0 + if (childRight >= parentRight) { + dx += childRight - parentRight + dx += parentWidth - childWidth - OFFSCREEN_ITEM_WIDTH + } else if (childLeft <= parentLeft) { + dx -= parentLeft - childLeft + dx -= parentWidth - childWidth - OFFSCREEN_ITEM_WIDTH + } + let x = parent.scrollLeft + dx + x = Math.max(0, x) + x = Math.min(x, parent.scrollWidth - parentWidth) + if (dx !== 0) { + parent.scroll({ + left: x, + behavior: 'smooth', + }) + } + }, [scrollElRef, selectedPage, styles]) + + const onPressItem = useCallback( + (index: number) => { + onSelect?.(index) + if (index === selectedPage) { + onPressSelected?.(index) + } + }, + [onSelect, selectedPage, onPressSelected], + ) + + return ( + + + {items.map((item, i) => { + const selected = i === selectedPage + return ( + (itemRefs.current[i] = node as any)} + style={styles.item} + hoverStyle={pal.viewLight} + onPress={() => onPressItem(i)} + accessibilityRole="tab"> + + + {item} + + + + ) + })} + + + + ) +} + +const desktopStyles = StyleSheet.create({ + outer: { + flexDirection: 'row', + width: 598, + }, + contentContainer: { + paddingHorizontal: 0, + backgroundColor: 'transparent', + }, + item: { + paddingTop: 14, + paddingHorizontal: 14, + justifyContent: 'center', + }, + itemInner: { + paddingBottom: 12, + borderBottomWidth: 3, + borderBottomColor: 'transparent', + }, + outerBottomBorder: { + position: 'absolute', + left: 0, + right: 0, + top: '100%', + borderBottomWidth: StyleSheet.hairlineWidth, + }, +}) + +const mobileStyles = StyleSheet.create({ + outer: { + flexDirection: 'row', + }, + contentContainer: { + backgroundColor: 'transparent', + paddingHorizontal: 6, + }, + item: { + paddingTop: 10, + paddingHorizontal: 10, + justifyContent: 'center', + }, + itemInner: { + paddingBottom: 10, + borderBottomWidth: 3, + borderBottomColor: 'transparent', + }, + outerBottomBorder: { + position: 'absolute', + left: 0, + right: 0, + top: '100%', + borderBottomWidth: StyleSheet.hairlineWidth, + }, +}) diff --git a/src/view/screens/Home.tsx b/src/view/screens/Home.tsx index 91c9ae69abb..7bd0b6e576f 100644 --- a/src/view/screens/Home.tsx +++ b/src/view/screens/Home.tsx @@ -11,7 +11,7 @@ import { HomeTabNavigatorParams, NativeStackScreenProps, } from '#/lib/routes/types' -import {logEvent, LogEvents} from '#/lib/statsig/statsig' +import {logEvent} from '#/lib/statsig/statsig' import {isWeb} from '#/platform/detection' import {emitSoftReset} from '#/state/events' import {SavedFeedSourceInfo, usePinnedFeedsInfos} from '#/state/queries/feed' @@ -121,7 +121,7 @@ function HomeScreenReady({ // This is supposed to only happen on the web when you use the right nav. if (selectedIndex !== lastPagerReportedIndexRef.current) { lastPagerReportedIndexRef.current = selectedIndex - pagerRef.current?.setPage(selectedIndex, 'desktop-sidebar-click') + pagerRef.current?.setPage(selectedIndex) } }, [selectedIndex]) @@ -158,21 +158,13 @@ function HomeScreenReady({ const feed = allFeeds[index] setSelectedFeed(feed) lastPagerReportedIndexRef.current = index - }, - [setDrawerSwipeDisabled, setSelectedFeed, setMinimalShellMode, allFeeds], - ) - - const onPageSelecting = React.useCallback( - (index: number, reason: LogEvents['home:feedDisplayed']['reason']) => { - const feed = allFeeds[index] logEvent('home:feedDisplayed', { index, feedType: feed.split('|')[0], feedUrl: feed, - reason, }) }, - [allFeeds], + [setDrawerSwipeDisabled, setSelectedFeed, setMinimalShellMode, allFeeds], ) const onPressSelected = React.useCallback(() => { @@ -228,7 +220,6 @@ function HomeScreenReady({ ref={pagerRef} testID="homeScreen" initialPage={selectedIndex} - onPageSelecting={onPageSelecting} onPageSelected={onPageSelected} onPageScrollStateChanged={onPageScrollStateChanged} renderTabBar={renderTabBar}>