-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Expenses list, FlashList #61185
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
Expenses list, FlashList #61185
Changes from all commits
5a26daa
bf10fe9
a40364e
aa0be7c
cd09ef0
186c4d8
a7a7a7a
abb887f
d7d5056
f65d798
70b8390
8bdcbf6
3a4ff6d
10c0a43
481ec6a
4491326
21d9401
c622ffe
084e165
2b2132d
e22fc29
960f88c
8ab6f83
3444898
4a1ee27
b0e85f6
2f03c88
d9a23af
3ad3e93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| import {useIsFocused} from '@react-navigation/native'; | ||
| import {FlashList} from '@shopify/flash-list'; | ||
| import type {FlashListProps, ViewToken} from '@shopify/flash-list'; | ||
| import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useRef, useState} from 'react'; | ||
| import type {ForwardedRef} from 'react'; | ||
| import {View} from 'react-native'; | ||
| import type {FlatList, ListRenderItemInfo, NativeSyntheticEvent, StyleProp, ViewStyle, ViewToken} from 'react-native'; | ||
| import type {NativeSyntheticEvent, StyleProp, ViewStyle} from 'react-native'; | ||
| import {useOnyx} from 'react-native-onyx'; | ||
| import Animated from 'react-native-reanimated'; | ||
| import type {FlatListPropsWithLayout} from 'react-native-reanimated'; | ||
| import Checkbox from '@components/Checkbox'; | ||
| import * as Expensicons from '@components/Icon/Expensicons'; | ||
| import MenuItem from '@components/MenuItem'; | ||
|
|
@@ -28,9 +29,11 @@ import useThemeStyles from '@hooks/useThemeStyles'; | |
| import {turnOffMobileSelectionMode, turnOnMobileSelectionMode} from '@libs/actions/MobileSelectionMode'; | ||
| import {isMobileChrome} from '@libs/Browser'; | ||
| import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener'; | ||
| import {isReportActionListItemType, isReportListItemType, isTransactionListItemType} from '@libs/SearchUIUtils'; | ||
| import variables from '@styles/variables'; | ||
| import CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import ITEM_HEIGHTS from './itemHeights'; | ||
|
|
||
| type SearchListItem = TransactionListItemType | ReportListItemType | ReportActionListItemType | TaskListItemType; | ||
| type SearchListItemComponentType = typeof TransactionListItem | typeof ChatListItem | typeof ReportListItem | typeof TaskListItem; | ||
|
|
@@ -40,7 +43,7 @@ type SearchListHandle = { | |
| scrollToIndex: (index: number, animated?: boolean) => void; | ||
| }; | ||
|
|
||
| type SearchListProps = Pick<FlatListPropsWithLayout<SearchListItem>, 'onScroll' | 'contentContainerStyle' | 'onEndReached' | 'onEndReachedThreshold' | 'ListFooterComponent'> & { | ||
| type SearchListProps = Pick<FlashListProps<SearchListItem>, 'onScroll' | 'contentContainerStyle' | 'onEndReached' | 'onEndReachedThreshold' | 'ListFooterComponent' | 'estimatedItemSize'> & { | ||
| data: SearchListItem[]; | ||
|
|
||
| /** Default renderer for every item in the list */ | ||
|
|
@@ -72,6 +75,9 @@ type SearchListProps = Pick<FlatListPropsWithLayout<SearchListItem>, 'onScroll' | |
| /** The hash of the queryJSON */ | ||
| queryJSONHash: number; | ||
|
|
||
| /** The type of the queryJSON */ | ||
| queryJSONType: string; | ||
|
|
||
| /** Whether to group the list by reports */ | ||
| shouldGroupByReports?: boolean; | ||
|
|
||
|
|
@@ -82,7 +88,8 @@ type SearchListProps = Pick<FlatListPropsWithLayout<SearchListItem>, 'onScroll' | |
| onLayout?: () => void; | ||
| }; | ||
|
|
||
| const onScrollToIndexFailed = () => {}; | ||
| const AnimatedFlashList = Animated.createAnimatedComponent<FlashListProps<SearchListItem>>(FlashList); | ||
| const keyExtractor = (item: SearchListItem, index: number) => item.keyForList ?? `${index}`; | ||
|
|
||
| function SearchList( | ||
| { | ||
|
|
@@ -104,7 +111,9 @@ function SearchList( | |
| queryJSONHash, | ||
| shouldGroupByReports, | ||
| onViewableItemsChanged, | ||
| estimatedItemSize = ITEM_HEIGHTS.NARROW_WITHOUT_DRAWER.STANDARD, | ||
| onLayout, | ||
| queryJSONType, | ||
| }: SearchListProps, | ||
| ref: ForwardedRef<SearchListHandle>, | ||
| ) { | ||
|
|
@@ -115,7 +124,7 @@ function SearchList( | |
| }, 0); | ||
| const {translate} = useLocalize(); | ||
| const isFocused = useIsFocused(); | ||
| const listRef = useRef<FlatList<SearchListItem>>(null); | ||
| const listRef = useRef<FlashList<SearchListItem>>(null); | ||
| const hasKeyBeenPressed = useRef(false); | ||
| const [itemsToHighlight, setItemsToHighlight] = useState<Set<string> | null>(null); | ||
| const itemFocusTimeoutRef = useRef<NodeJS.Timeout | null>(null); | ||
|
|
@@ -124,7 +133,7 @@ function SearchList( | |
| // We need to use isSmallScreenWidth instead of shouldUseNarrowLayout here because there is a race condition that causes shouldUseNarrowLayout to change indefinitely in this component | ||
| // See https://github.com/Expensify/App/issues/48675 for more details | ||
| // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth | ||
| const {isSmallScreenWidth} = useResponsiveLayout(); | ||
| const {isSmallScreenWidth, isLargeScreenWidth, shouldUseNarrowLayout} = useResponsiveLayout(); | ||
|
|
||
| const [isModalVisible, setIsModalVisible] = useState(false); | ||
| const {selectionMode} = useMobileSelectionMode(); | ||
|
|
@@ -299,8 +308,77 @@ function SearchList( | |
|
|
||
| useImperativeHandle(ref, () => ({scrollAndHighlightItem, scrollToIndex}), [scrollAndHighlightItem, scrollToIndex]); | ||
|
|
||
| const getItemHeight = useCallback( | ||
| (item: SearchListItem): number => { | ||
| try { | ||
| const reportListItem = item as ReportListItemType; | ||
| const transactionListItem = item as TransactionListItemType; | ||
| const reportActionListItem = item as ReportActionListItemType; | ||
|
luacmartins marked this conversation as resolved.
Comment on lines
+314
to
+316
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This casting leads to very poor type safety. The types of the variables The code for now works, because you have your checks and the optional chaining correctly place, but this is not enforced correctly by the type checks, another dev can easily do things wrong trusting the types of these variables when they are not what they seem. A better approach would have been something like: or something like: The later starts showing a type error which makes me question if there is a bug here or if it is really intentional that you want to return
Is the behaviour correct? @perunt Please please please avoid coding things using casting unless it is really necessary (not sure if it is ever necessary), it leads to brittle code since types are not reliable anymore.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that's a nice improvement. I'll also keep an eye out for these casts in the future. @perunt let's update this code to ensure better type safety
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey. I agree that it was not the best decision. I changed it here: #63236. @luacmartins can you take it?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like that PR is merged. Thanks for the quick work!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for improving it! |
||
|
|
||
| const isTransaction = isTransactionListItemType(transactionListItem); | ||
| const isReportAction = isReportActionListItemType(reportActionListItem); | ||
|
|
||
| if (isTransaction || isReportAction) { | ||
| if (queryJSONType === CONST.SEARCH.DATA_TYPES.CHAT) { | ||
| return reportListItem?.childReportID ? variables.searchListItemHeightChat : variables.searchListItemHeightChatCompact; | ||
| } | ||
| const itemAction = transactionListItem?.action; | ||
| // VIEW is the only action type that should be compact | ||
| const isItemActionView = isTransaction && itemAction === CONST.SEARCH.ACTION_TYPES.VIEW; | ||
|
|
||
| // Determine which layout to use based on screen size and drawer state | ||
| let heightConstants; | ||
|
|
||
| if (shouldUseNarrowLayout) { | ||
| // For narrow screens without drawer (mobile or collapsed desktop) | ||
| heightConstants = isItemActionView ? ITEM_HEIGHTS.NARROW_WITHOUT_DRAWER.STANDARD : ITEM_HEIGHTS.NARROW_WITHOUT_DRAWER.WITH_BUTTON; | ||
| } else if (!isLargeScreenWidth) { | ||
| // For narrow screens with drawer | ||
| heightConstants = isItemActionView ? ITEM_HEIGHTS.NARROW_WITH_DRAWER.STANDARD : ITEM_HEIGHTS.NARROW_WITH_DRAWER.WITH_BUTTON; | ||
| } else { | ||
| // For wide screens (desktop) | ||
| heightConstants = ITEM_HEIGHTS.WIDE.STANDARD; | ||
| } | ||
|
|
||
| return heightConstants; | ||
| } | ||
| if (isReportListItemType(reportListItem)) { | ||
| if (!reportListItem.transactions || reportListItem.transactions.length === 0) { | ||
| return Math.max(ITEM_HEIGHTS.HEADER, 1); | ||
| } | ||
| const baseReportItemHeight = isLargeScreenWidth | ||
| ? variables.searchOptionRowMargin + variables.searchOptionRowBaseHeight + variables.searchOptionRowLargeFooterHeight | ||
| : variables.searchOptionRowMargin + variables.searchOptionRowBaseHeight + variables.searchOptionRowSmallFooterHeight; | ||
| const transactionHeight = variables.searchOptionRowTransactionHeight; | ||
| const calculatedHeight = | ||
| baseReportItemHeight + reportListItem.transactions.length * transactionHeight + variables.optionRowListItemPadding + variables.searchOptionRowMargin; | ||
| return Math.max(calculatedHeight, ITEM_HEIGHTS.HEADER, 1); | ||
| } | ||
|
|
||
| return isLargeScreenWidth ? variables.searchListItemHeightLargeScreen : variables.searchListItemHeightSmallScreen; | ||
| } catch (error) { | ||
| console.error('SearchList: Error calculating item height, returning estimated size.', error, item); | ||
| return estimatedItemSize; | ||
| } | ||
| }, | ||
| [isLargeScreenWidth, estimatedItemSize, shouldUseNarrowLayout, queryJSONType], | ||
| ); | ||
|
|
||
| const overrideItemLayout = useCallback( | ||
| (layout: {span?: number; size?: number}, item: SearchListItem) => { | ||
| const height = getItemHeight(item); | ||
| if (!layout) { | ||
| return; | ||
| } | ||
| // eslint-disable-next-line no-param-reassign | ||
| layout.size = height > 0 ? height : estimatedItemSize; | ||
| }, | ||
| [getItemHeight, estimatedItemSize], | ||
| ); | ||
|
|
||
| const renderItem = useCallback( | ||
| ({item, index}: ListRenderItemInfo<SearchListItem>) => { | ||
| // eslint-disable-next-line react/no-unused-prop-types | ||
| ({item, index}: {item: SearchListItem; index: number}) => { | ||
| const isItemFocused = focusedIndex === index; | ||
| const isItemHighlighted = !!itemsToHighlight?.has(item.keyForList ?? ''); | ||
|
|
||
|
|
@@ -386,22 +464,23 @@ function SearchList( | |
| )} | ||
| </View> | ||
| )} | ||
|
|
||
| <Animated.FlatList | ||
| <AnimatedFlashList | ||
| ref={listRef} | ||
| data={data} | ||
| renderItem={renderItem} | ||
| keyExtractor={(item, index) => item.keyForList ?? `${index}`} | ||
| keyExtractor={keyExtractor} | ||
| onScroll={onScroll} | ||
| contentContainerStyle={contentContainerStyle} | ||
| showsVerticalScrollIndicator={false} | ||
| ref={listRef} | ||
| extraData={focusedIndex} | ||
| estimatedItemSize={estimatedItemSize} | ||
| overrideItemLayout={overrideItemLayout} | ||
| onEndReached={onEndReached} | ||
| onEndReachedThreshold={onEndReachedThreshold} | ||
| ListFooterComponent={ListFooterComponent} | ||
| drawDistance={1000} | ||
| extraData={focusedIndex} | ||
| removeClippedSubviews | ||
| onViewableItemsChanged={onViewableItemsChanged} | ||
| onScrollToIndexFailed={onScrollToIndexFailed} | ||
| onLayout={onLayout} | ||
| /> | ||
| <Modal | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import variables from '@styles/variables'; | ||
|
|
||
| const ITEM_HEIGHTS = { | ||
| // Constants for wide screen layout | ||
| WIDE: { | ||
| STANDARD: variables.optionRowWideItemHeight + variables.optionRowListItemPadding, | ||
| }, | ||
|
|
||
| // Constants for narrow screen with drawer | ||
| NARROW_WITH_DRAWER: { | ||
| STANDARD: variables.optionRowNarrowWithDrawerItemHeight + variables.optionRowListItemPadding, | ||
| WITH_BUTTON: variables.optionRowNarrowWithDrawerItemHeightWithButton + variables.optionRowListItemPadding, | ||
| }, | ||
|
|
||
| // Constants for narrow screen without drawer (mobile-like) | ||
| NARROW_WITHOUT_DRAWER: { | ||
| STANDARD: variables.optionRowNarrowWithoutDrawerItemHeight + variables.optionRowListItemPadding, | ||
| WITH_BUTTON: variables.optionRowNarrowWithoutDrawerItemHeightWithButton + variables.optionRowListItemPadding, | ||
| }, | ||
|
|
||
| HEADER: variables.optionRowSearchHeaderHeight, | ||
| } as const; | ||
|
|
||
| export default ITEM_HEIGHTS; |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means that if we add a new list item, we'd have to add it here too right? Is there a more foolproof way of doing this? Maybe we could use the same pattern we have for sections, e.g. here, so that it's centralized in SearchUIUtils and this component just calls
getItemHeight(type, ...). I think that'd make it easier to see all the places that we need to update when adding a new list item typeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we can architect something neater. I have a few concerns about putting it away since this value will always recalculate on resizing or when new data comes in. This may cause some loading effects.
The second thing is that our items sometimes have three different sizes (wide screen with drawer, small screen with drawer, small screen) and sometimes it's messy and undesired. If we can unify some sizes of items, we can drastically simplify it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, let's clean this up in a follow up. I think that we can make it easier to scale as we add more items to Reports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note - @luacmartins When we switch to FlashList v2, we can remove this size estimation logic entirely.