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

Fix wrong scrolling in Search list when navigating by keyboard #43490

31 changes: 30 additions & 1 deletion src/components/Search.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import {useNavigation} from '@react-navigation/native';
import type {StackNavigationProp} from '@react-navigation/stack';
import React, {useEffect, useRef} from 'react';
import React, {useCallback, useEffect, useRef} from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import {useOnyx} from 'react-native-onyx';
import useNetwork from '@hooks/useNetwork';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as SearchActions from '@libs/actions/Search';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import Log from '@libs/Log';
Expand All @@ -14,6 +15,7 @@ import type {SearchColumnType, SortOrder} from '@libs/SearchUtils';
import Navigation from '@navigation/Navigation';
import type {CentralPaneNavigatorParamList} from '@navigation/types';
import EmptySearchView from '@pages/Search/EmptySearchView';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
Expand All @@ -34,6 +36,10 @@ type SearchProps = {
};

const sortableSearchTabs: SearchQuery[] = [CONST.TAB_SEARCH.ALL];
const transactionItemMobileHeight = 100;
const reportItemTransactionHeight = 52;
const listItemPadding = 12; // this is equivalent to 'mb3' on every transaction/report list item
Comment on lines +39 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB This feels brittle since if the height changes we would introduce the bug again. That being said, I'm not sure how to solve it this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly same for me. We could in some way extract the same variable from styles perhaps and use, but still you would have to change it in two places.

I agree that its brittle but I see no other simple solution. padding affects item height, and correct item height has to be pushed down to RN List

const searchHeaderHeight = 54;

function isTransactionListItemType(item: TransactionListItemType | ReportListItemType): item is TransactionListItemType {
const transactionListItem = item as TransactionListItemType;
Expand All @@ -43,9 +49,30 @@ function isTransactionListItemType(item: TransactionListItemType | ReportListIte
function Search({query, policyIDs, sortBy, sortOrder}: SearchProps) {
const {isOffline} = useNetwork();
const styles = useThemeStyles();
const {isLargeScreenWidth} = useWindowDimensions();
const navigation = useNavigation<StackNavigationProp<CentralPaneNavigatorParamList>>();
const lastSearchResultsRef = useRef<OnyxEntry<SearchResults>>();

const getItemHeight = useCallback(
(item: TransactionListItemType | ReportListItemType) => {
if (isTransactionListItemType(item)) {
return isLargeScreenWidth ? variables.optionRowHeight + listItemPadding : transactionItemMobileHeight + listItemPadding;
}

if (item.transactions.length === 0) {
return 0;
}

if (item.transactions.length === 1) {
return isLargeScreenWidth ? variables.optionRowHeight + listItemPadding : transactionItemMobileHeight + listItemPadding;
}

const baseReportItemHeight = isLargeScreenWidth ? 72 : 108;
return baseReportItemHeight + item.transactions.length * reportItemTransactionHeight + listItemPadding;
},
[isLargeScreenWidth],
);

const hash = SearchUtils.getQueryHash(query, policyIDs, sortBy, sortOrder);
const [currentSearchResults, searchResultsMeta] = useOnyx(`${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`);

Expand Down Expand Up @@ -136,6 +163,7 @@ function Search({query, policyIDs, sortBy, sortOrder}: SearchProps) {
shouldShowYear={shouldShowYear}
/>
}
customListHeaderHeight={searchHeaderHeight}
// To enhance the smoothness of scrolling and minimize the risk of encountering blank spaces during scrolling,
// we have configured a larger windowSize and a longer delay between batch renders.
// The windowSize determines the number of items rendered before and after the currently visible items.
Expand All @@ -150,6 +178,7 @@ function Search({query, policyIDs, sortBy, sortOrder}: SearchProps) {
ListItem={ListItem}
sections={[{data: sortedData, isDisabled: false}]}
onSelectRow={(item) => openReport(item)}
getItemHeight={getItemHeight}
shouldDebounceRowSelect
shouldPreventDefaultFocusOnSelectRow={!DeviceCapabilities.canUseTouchScreen()}
listHeaderWrapperStyle={[styles.ph9, styles.pv3, styles.pb5]}
Expand Down
11 changes: 8 additions & 3 deletions src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import {isEmptyObject} from '@src/types/utils/EmptyObject';
import arraysEqual from '@src/utils/arraysEqual';
import type {BaseSelectionListProps, ButtonOrCheckBoxRoles, FlattenedSectionsReturn, ListItem, SectionListDataType, SectionWithIndexOffset, SelectionListHandle} from './types';

const getDefaultItemHeight = () => variables.optionRowHeight;

function BaseSelectionList<TItem extends ListItem>(
{
sections,
Expand All @@ -40,6 +42,7 @@ function BaseSelectionList<TItem extends ListItem>(
onCheckboxPress,
onSelectAll,
onDismissError,
getItemHeight = getDefaultItemHeight,
textInputLabel = '',
textInputPlaceholder = '',
textInputValue = '',
Expand Down Expand Up @@ -71,6 +74,7 @@ function BaseSelectionList<TItem extends ListItem>(
isLoadingNewOptions = false,
onLayout,
customListHeader,
customListHeaderHeight = 0,
listHeaderWrapperStyle,
isRowMultilineSupported = false,
textInputRef,
Expand Down Expand Up @@ -125,7 +129,8 @@ function BaseSelectionList<TItem extends ListItem>(
const disabledArrowKeyOptionsIndexes: number[] = [];
let disabledIndex = 0;

let offset = 0;
// need to account that the list might have some extra content above it
let offset = customListHeader ? customListHeaderHeight : 0;
const itemLayouts = [{length: 0, offset}];

const selectedOptions: TItem[] = [];
Expand Down Expand Up @@ -155,7 +160,7 @@ function BaseSelectionList<TItem extends ListItem>(
disabledIndex += 1;

// Account for the height of the item in getItemLayout
const fullItemHeight = variables.optionRowHeight;
const fullItemHeight = getItemHeight(item);
itemLayouts.push({length: fullItemHeight, offset});
offset += fullItemHeight;

Expand Down Expand Up @@ -187,7 +192,7 @@ function BaseSelectionList<TItem extends ListItem>(
itemLayouts,
allSelected: selectedOptions.length > 0 && selectedOptions.length === allOptions.length - disabledOptionsIndexes.length,
};
}, [canSelectMultiple, sections]);
}, [canSelectMultiple, sections, customListHeader, customListHeaderHeight, getItemHeight]);

const [slicedSections, ShowMoreButtonInstance] = useMemo(() => {
let remainingOptionsLimit = CONST.MAX_SELECTION_LIST_PAGE_LENGTH * currentPage;
Expand Down
9 changes: 9 additions & 0 deletions src/components/SelectionList/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,12 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
/** Callback to fire when "Select All" checkbox is pressed. Only use along with `canSelectMultiple` */
onSelectAll?: () => void;

/**
* Callback that should return height of the specific item
* Only use this if we're handling some non-standard items, most of the time the default value is correct
*/
getItemHeight?: (item: TItem) => number;

/** Callback to fire when an error is dismissed */
onDismissError?: (item: TItem) => void;

Expand Down Expand Up @@ -390,6 +396,9 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
/** Custom header to show right above list */
customListHeader?: ReactNode;

/** When customListHeader is provided, this should be its height needed for correct list scrolling */
customListHeaderHeight?: number;

/** Styles for the list header wrapper */
listHeaderWrapperStyle?: StyleProp<ViewStyle>;

Expand Down
4 changes: 3 additions & 1 deletion src/libs/SearchUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,11 @@ function getReportSections(data: OnyxTypes.SearchResults['data']): ReportListIte
if (key.startsWith(ONYXKEYS.COLLECTION.REPORT)) {
const value = {...data[key]};
const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${value.reportID}`;
const transactions = reportIDToTransactions[reportKey]?.transactions ?? [];

reportIDToTransactions[reportKey] = {
...value,
transactions: reportIDToTransactions[reportKey]?.transactions ?? [],
transactions,
};
} else if (key.startsWith(ONYXKEYS.COLLECTION.TRANSACTION)) {
const transactionItem = {...data[key]};
Expand Down
Loading