-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Decompose SearchPage: extract SearchFilterSync and useReceiptScanDrop #83602
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import {useEffect} from 'react'; | ||
| import type {SearchQueryJSON} from '@components/Search/types'; | ||
| import useFilterFormValues from '@hooks/useFilterFormValues'; | ||
| import {updateAdvancedFilters} from '@libs/actions/Search'; | ||
|
|
||
| type SearchFilterSyncProps = { | ||
| queryJSON?: SearchQueryJSON; | ||
| }; | ||
|
|
||
| /** | ||
| * Component that does not render anything but owns the filter-form Onyx subscriptions (~6 keys) | ||
| * and syncs them to the advanced filters form whenever the query changes. | ||
| * Extracted from SearchPage to isolate re-renders caused by these subscriptions. | ||
| */ | ||
| function SearchFilterSync({queryJSON}: SearchFilterSyncProps) { | ||
| const formValues = useFilterFormValues(queryJSON); | ||
|
|
||
| useEffect(() => { | ||
| updateAdvancedFilters(formValues, true); | ||
| }, [formValues]); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| export default SearchFilterSync; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,59 +1,42 @@ | ||
| import React, {useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react'; | ||
| import type {NativeScrollEvent, NativeSyntheticEvent} from 'react-native'; | ||
| import Animated from 'react-native-reanimated'; | ||
| import DragAndDropConsumer from '@components/DragAndDrop/Consumer'; | ||
| import DragAndDropProvider from '@components/DragAndDrop/Provider'; | ||
| import DropZoneUI from '@components/DropZone/DropZoneUI'; | ||
| import {ScrollOffsetContext} from '@components/ScrollOffsetContextProvider'; | ||
| import {useSearchActionsContext, useSearchStateContext} from '@components/Search/SearchContext'; | ||
| import type {SearchParams} from '@components/Search/types'; | ||
| import {usePlaybackActionsContext} from '@components/VideoPlayerContexts/PlaybackContext'; | ||
| import useConfirmReadyToOpenApp from '@hooks/useConfirmReadyToOpenApp'; | ||
| import useFilterFormValues from '@hooks/useFilterFormValues'; | ||
| import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; | ||
| import useLocalize from '@hooks/useLocalize'; | ||
| import useMobileSelectionMode from '@hooks/useMobileSelectionMode'; | ||
| import usePrevious from '@hooks/usePrevious'; | ||
| import useReceiptScanDrop from '@hooks/useReceiptScanDrop'; | ||
| import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||
| import useSearchShouldCalculateTotals from '@hooks/useSearchShouldCalculateTotals'; | ||
| import useTheme from '@hooks/useTheme'; | ||
| import useThemeStyles from '@hooks/useThemeStyles'; | ||
| import {searchInServer} from '@libs/actions/Report'; | ||
| import {search, updateAdvancedFilters} from '@libs/actions/Search'; | ||
| import {search} from '@libs/actions/Search'; | ||
| import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; | ||
| import type {SearchFullscreenNavigatorParamList} from '@libs/Navigation/types'; | ||
| import {buildSearchQueryJSON} from '@libs/SearchQueryUtils'; | ||
| import variables from '@styles/variables'; | ||
| import CONST from '@src/CONST'; | ||
| import type SCREENS from '@src/SCREENS'; | ||
| import type {SearchResults} from '@src/types/onyx'; | ||
| import SearchFilterSync from './SearchFilterSync'; | ||
| import SearchPageNarrow from './SearchPageNarrow'; | ||
| import SearchPageWide from './SearchPageWide'; | ||
|
|
||
| type SearchPageProps = PlatformStackScreenProps<SearchFullscreenNavigatorParamList, typeof SCREENS.SEARCH.ROOT>; | ||
|
|
||
| function SearchPage({route}: SearchPageProps) { | ||
| const {translate} = useLocalize(); | ||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const styles = useThemeStyles(); | ||
| const theme = useTheme(); | ||
| const {selectedTransactions, lastSearchType, areAllMatchingItemsSelected, currentSearchKey, currentSearchResults} = useSearchStateContext(); | ||
| const {clearSelectedTransactions, setLastSearchType} = useSearchActionsContext(); | ||
| const isMobileSelectionModeEnabled = useMobileSelectionMode(clearSelectedTransactions); | ||
|
|
||
| const queryJSON = useMemo(() => buildSearchQueryJSON(route.params.q, route.params.rawQuery), [route.params.q, route.params.rawQuery]); | ||
| const {saveScrollOffset} = useContext(ScrollOffsetContext); | ||
| const expensifyIcons = useMemoizedLazyExpensifyIcons(['SmartScan'] as const); | ||
|
|
||
| const lastNonEmptySearchResults = useRef<SearchResults | undefined>(undefined); | ||
|
|
||
| const formValues = useFilterFormValues(queryJSON); | ||
|
|
||
| useEffect(() => { | ||
| updateAdvancedFilters(formValues, true); | ||
| }, [formValues]); | ||
|
|
||
| useConfirmReadyToOpenApp(); | ||
|
|
||
| useEffect(() => { | ||
|
|
@@ -69,7 +52,6 @@ function SearchPage({route}: SearchPageProps) { | |
|
|
||
| const selectedTransactionsKeys = Object.keys(selectedTransactions ?? {}); | ||
|
|
||
| const {initScanRequest, PDFValidationComponent, ErrorModal, isDragDisabled} = useReceiptScanDrop(); | ||
| const {resetVideoPlayerData} = usePlaybackActionsContext(); | ||
|
|
||
| const [isSorting, setIsSorting] = useState(false); | ||
|
|
@@ -160,29 +142,16 @@ function SearchPage({route}: SearchPageProps) { | |
|
|
||
| return ( | ||
| <Animated.View style={[styles.flex1]}> | ||
| <SearchFilterSync queryJSON={queryJSON} /> | ||
|
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. Do we need this call in SearchPage? Can we move this to
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. Yeah, as low as it makes sense. Filters were all different since I started here.
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. Yea, I think we can remove it from SearchPage and centralize it in SearchFiltersBar. We should test for any adverse side effects though |
||
| {shouldUseNarrowLayout ? ( | ||
| <DragAndDropProvider isDisabled={isDragDisabled}> | ||
| {PDFValidationComponent} | ||
| <SearchPageNarrow | ||
| queryJSON={queryJSON} | ||
| metadata={metadata} | ||
| searchResults={searchResults} | ||
| isMobileSelectionModeEnabled={isMobileSelectionModeEnabled} | ||
| footerData={footerData} | ||
| shouldShowFooter={shouldShowFooter} | ||
| /> | ||
| <DragAndDropConsumer onDrop={initScanRequest}> | ||
| <DropZoneUI | ||
| icon={expensifyIcons.SmartScan} | ||
| dropTitle={translate('dropzone.scanReceipts')} | ||
| dropStyles={styles.receiptDropOverlay(true)} | ||
| dropTextStyles={styles.receiptDropText} | ||
| dropWrapperStyles={{marginBottom: variables.bottomTabHeight}} | ||
| dashedBorderStyles={[styles.dropzoneArea, styles.easeInOpacityTransition, styles.activeDropzoneDashedBorder(theme.receiptDropBorderColorActive, true)]} | ||
| /> | ||
| </DragAndDropConsumer> | ||
| {ErrorModal} | ||
| </DragAndDropProvider> | ||
| <SearchPageNarrow | ||
| queryJSON={queryJSON} | ||
| metadata={metadata} | ||
| searchResults={searchResults} | ||
| isMobileSelectionModeEnabled={isMobileSelectionModeEnabled} | ||
| footerData={footerData} | ||
| shouldShowFooter={shouldShowFooter} | ||
| /> | ||
| ) : ( | ||
| <SearchPageWide | ||
| queryJSON={queryJSON} | ||
|
|
@@ -193,10 +162,6 @@ function SearchPage({route}: SearchPageProps) { | |
| handleSearchAction={handleSearchAction} | ||
| onSortPressedCallback={onSortPressedCallback} | ||
| scrollHandler={scrollHandler} | ||
| initScanRequest={initScanRequest} | ||
| isDragDisabled={isDragDisabled} | ||
| PDFValidationComponent={PDFValidationComponent} | ||
| ErrorModal={ErrorModal} | ||
| shouldShowFooter={shouldShowFooter} | ||
| /> | ||
| )} | ||
|
|
||
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.
Do we need this component? Can we just delegate the sync to
SearchFiltersBarwhich already subscribed to theuseFilterFormValueshook?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.
if it has to access all of these internal state variables (onyx), it could do it yes. can we decouple the Filters component from these subscriptions though or do you think that's not possible?
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.
I think we can move this logic to the SearchFiltersBar only, so we don't need this separate component here
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.
That way we'll rerender filters though on all internal subs of this hook