-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Reports page and TransactionItemRow performance improvements[part 1] #65421
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
Changes from all commits
4bf02fb
9384ded
3d004eb
7f416c6
196c2a7
572bdde
3eeb275
d98d2da
3fbda01
b4141c0
3029843
79795df
f8fddc9
53c9ce8
af258b3
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 |
|---|---|---|
|
|
@@ -20,12 +20,11 @@ import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager'; | |
| import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; | ||
| import useKeyboardState from '@hooks/useKeyboardState'; | ||
| import useLocalize from '@hooks/useLocalize'; | ||
| import useMobileSelectionMode from '@hooks/useMobileSelectionMode'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
| import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||
| import useSafeAreaPaddings from '@hooks/useSafeAreaPaddings'; | ||
| import useThemeStyles from '@hooks/useThemeStyles'; | ||
| import {turnOffMobileSelectionMode, turnOnMobileSelectionMode} from '@libs/actions/MobileSelectionMode'; | ||
| import {turnOnMobileSelectionMode} from '@libs/actions/MobileSelectionMode'; | ||
| import {isMobileChrome} from '@libs/Browser'; | ||
| import {addKeyDownPressListener, removeKeyDownPressListener} from '@libs/KeyboardShortcut/KeyDownPressListener'; | ||
| import variables from '@styles/variables'; | ||
|
|
@@ -78,6 +77,13 @@ type SearchListProps = Pick<FlatListPropsWithLayout<SearchListItem>, 'onScroll' | |
|
|
||
| /** Invoked on mount and layout changes */ | ||
| onLayout?: () => void; | ||
|
|
||
| /** Whether mobile selection mode is enabled */ | ||
| isMobileSelectionModeEnabled: boolean; | ||
| }; | ||
|
|
||
| const keyExtractor = (item: SearchListItem, index: number) => { | ||
| return item.keyForList ?? `${index}`; | ||
| }; | ||
|
|
||
| const onScrollToIndexFailed = () => {}; | ||
|
|
@@ -102,6 +108,7 @@ function SearchList( | |
| queryJSON, | ||
| onViewableItemsChanged, | ||
| onLayout, | ||
| isMobileSelectionModeEnabled, | ||
| }: SearchListProps, | ||
| ref: ForwardedRef<SearchListHandle>, | ||
| ) { | ||
|
|
@@ -126,51 +133,14 @@ function SearchList( | |
| const {isSmallScreenWidth} = useResponsiveLayout(); | ||
|
|
||
| const [isModalVisible, setIsModalVisible] = useState(false); | ||
| const {selectionMode} = useMobileSelectionMode(); | ||
| const [longPressedItem, setLongPressedItem] = useState<SearchListItem>(); | ||
| // Check if selection should be on when the modal is opened | ||
| const wasSelectionOnRef = useRef(false); | ||
| // Keep track of the number of selected items to determine if we should turn off selection mode | ||
| const selectionRef = useRef(0); | ||
|
|
||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, { | ||
| canBeMissing: true, | ||
| }); | ||
|
|
||
| const [allReports] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {canBeMissing: false}); | ||
|
|
||
| useEffect(() => { | ||
| selectionRef.current = selectedItemsLength; | ||
|
|
||
| if (!isSmallScreenWidth) { | ||
| if (selectedItemsLength === 0) { | ||
| turnOffMobileSelectionMode(); | ||
| } | ||
| return; | ||
| } | ||
| if (!isFocused) { | ||
| return; | ||
| } | ||
| if (!wasSelectionOnRef.current && selectedItemsLength > 0) { | ||
| wasSelectionOnRef.current = true; | ||
| } | ||
| if (selectedItemsLength > 0 && !selectionMode?.isEnabled) { | ||
| turnOnMobileSelectionMode(); | ||
| } else if (selectedItemsLength === 0 && selectionMode?.isEnabled && !wasSelectionOnRef.current) { | ||
| turnOffMobileSelectionMode(); | ||
| } | ||
|
Comment on lines
-159
to
-161
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. Coming from the #66027 checklist: we’ve removed this logic, so in case the user navigates back using the Android back button, we need to call turnOffMobileSelectionMode |
||
| }, [selectionMode, isSmallScreenWidth, isFocused, selectedItemsLength]); | ||
|
|
||
| useEffect( | ||
| () => () => { | ||
| if (selectionRef.current !== 0) { | ||
| return; | ||
| } | ||
| turnOffMobileSelectionMode(); | ||
| }, | ||
| [], | ||
| ); | ||
|
|
||
|
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. amazing 👏
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.
|
||
| const handleLongPressRow = useCallback( | ||
| (item: SearchListItem) => { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
|
|
@@ -181,14 +151,14 @@ function SearchList( | |
| if ('transactions' in item && item.transactions.length === 0) { | ||
| return; | ||
| } | ||
| if (selectionMode?.isEnabled) { | ||
| if (isMobileSelectionModeEnabled) { | ||
| onCheckboxPress(item); | ||
| return; | ||
| } | ||
| setLongPressedItem(item); | ||
| setIsModalVisible(true); | ||
| }, | ||
| [isFocused, isSmallScreenWidth, onCheckboxPress, selectionMode?.isEnabled, shouldPreventLongPressRow], | ||
| [isFocused, isSmallScreenWidth, onCheckboxPress, isMobileSelectionModeEnabled, shouldPreventLongPressRow], | ||
| ); | ||
|
|
||
| const turnOnSelectionMode = useCallback(() => { | ||
|
|
@@ -398,7 +368,7 @@ function SearchList( | |
| <Animated.FlatList | ||
| data={data} | ||
| renderItem={renderItem} | ||
| keyExtractor={(item, index) => item.keyForList ?? `${index}`} | ||
| keyExtractor={keyExtractor} | ||
| onScroll={onScroll} | ||
| contentContainerStyle={contentContainerStyle} | ||
| showsVerticalScrollIndicator={false} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,9 +44,10 @@ import type {SearchHeaderOptionValue} from './SearchPageHeader'; | |
| type SearchFiltersBarProps = { | ||
| queryJSON: SearchQueryJSON; | ||
| headerButtonsOptions: Array<DropdownOption<SearchHeaderOptionValue>>; | ||
| isMobileSelectionModeEnabled: boolean; | ||
|
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. NAB: I'm not sure if we need to introduce this prop, since all the places using this component already pass the value from the useMobileSelectionMode hook
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. Each |
||
| }; | ||
|
|
||
| function SearchFiltersBar({queryJSON, headerButtonsOptions}: SearchFiltersBarProps) { | ||
| function SearchFiltersBar({queryJSON, headerButtonsOptions, isMobileSelectionModeEnabled}: SearchFiltersBarProps) { | ||
| const scrollRef = useRef<RNScrollView>(null); | ||
|
|
||
| // type, groupBy and status values are not guaranteed to respect the ts type as they come from user input | ||
|
|
@@ -70,7 +71,6 @@ function SearchFiltersBar({queryJSON, headerButtonsOptions}: SearchFiltersBarPro | |
| const [policyTagsLists] = useOnyx(ONYXKEYS.COLLECTION.POLICY_TAGS, {canBeMissing: true}); | ||
| const [policyCategories] = useOnyx(ONYXKEYS.COLLECTION.POLICY_CATEGORIES, {canBeMissing: true}); | ||
| const [workspaceCardFeeds] = useOnyx(ONYXKEYS.COLLECTION.WORKSPACE_CARDS_LIST, {canBeMissing: true}); | ||
| const [selectionMode] = useOnyx(ONYXKEYS.MOBILE_SELECTION_MODE, {canBeMissing: true}); | ||
| const [allFeeds] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER, {canBeMissing: true}); | ||
| const [searchResultsErrors] = useOnyx(`${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`, {canBeMissing: true, selector: (data) => data?.errors}); | ||
|
|
||
|
|
@@ -79,7 +79,7 @@ function SearchFiltersBar({queryJSON, headerButtonsOptions}: SearchFiltersBarPro | |
| const selectedTransactionsKeys = useMemo(() => Object.keys(selectedTransactions ?? {}), [selectedTransactions]); | ||
|
|
||
| const hasErrors = Object.keys(searchResultsErrors ?? {}).length > 0 && !isOffline; | ||
| const shouldShowSelectedDropdown = headerButtonsOptions.length > 0 && (!shouldUseNarrowLayout || (!!selectionMode && selectionMode.isEnabled)); | ||
| const shouldShowSelectedDropdown = headerButtonsOptions.length > 0 && (!shouldUseNarrowLayout || isMobileSelectionModeEnabled); | ||
|
|
||
| const [typeOptions, type] = useMemo(() => { | ||
| const options = getTypeOptions(allPolicies, email); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,9 @@ import {View} from 'react-native'; | |
| import type {DropdownOption} from '@components/ButtonWithDropdownMenu/types'; | ||
| import {useSearchContext} from '@components/Search/SearchContext'; | ||
| import type {SearchQueryJSON} from '@components/Search/types'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
| import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||
| import SearchSelectedNarrow from '@pages/Search/SearchSelectedNarrow'; | ||
| import type CONST from '@src/CONST'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type DeepValueOf from '@src/types/utils/DeepValueOf'; | ||
| import SearchPageHeaderInput from './SearchPageHeaderInput'; | ||
|
|
||
|
|
@@ -18,18 +16,26 @@ type SearchPageHeaderProps = { | |
| onSearchRouterFocus?: () => void; | ||
| headerButtonsOptions: Array<DropdownOption<SearchHeaderOptionValue>>; | ||
| handleSearch: (value: string) => void; | ||
| isMobileSelectionModeEnabled: boolean; | ||
|
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. NAB: same above all the places using this component already pass the value from the useMobileSelectionMode hook so i think we don't need to introduce this props
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. The same answear as above ☝️ |
||
| }; | ||
|
|
||
| type SearchHeaderOptionValue = DeepValueOf<typeof CONST.SEARCH.BULK_ACTION_TYPES> | undefined; | ||
|
|
||
| function SearchPageHeader({queryJSON, searchRouterListVisible, hideSearchRouterList, onSearchRouterFocus, headerButtonsOptions, handleSearch}: SearchPageHeaderProps) { | ||
| function SearchPageHeader({ | ||
| queryJSON, | ||
| searchRouterListVisible, | ||
| hideSearchRouterList, | ||
| onSearchRouterFocus, | ||
| headerButtonsOptions, | ||
| handleSearch, | ||
| isMobileSelectionModeEnabled, | ||
| }: SearchPageHeaderProps) { | ||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const {selectedTransactions} = useSearchContext(); | ||
| const [selectionMode] = useOnyx(ONYXKEYS.MOBILE_SELECTION_MODE, {canBeMissing: true}); | ||
|
|
||
| const selectedTransactionsKeys = Object.keys(selectedTransactions ?? {}); | ||
|
|
||
| if (shouldUseNarrowLayout && selectionMode?.isEnabled) { | ||
| if (shouldUseNarrowLayout && isMobileSelectionModeEnabled) { | ||
| return ( | ||
| <View> | ||
| <SearchSelectedNarrow | ||
|
|
||
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.
Did you remove this type as its not used anymore?