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

Item in report list is not highlighted and list cannot be navigated with keyboard #37081

Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
79f02bc
Fix bug with Item in report list is not highlighted and list cannot b…
ZhenjaHorbach Feb 22, 2024
172c807
Fix TS issue and update tests
ZhenjaHorbach Feb 22, 2024
f953b79
Merge branch 'main' into item-in-report-list-is-not-highlighted-and-l…
ZhenjaHorbach Feb 22, 2024
41f1507
Update branch
ZhenjaHorbach Feb 22, 2024
049756b
Update tests
ZhenjaHorbach Feb 22, 2024
150c90b
Update branch
ZhenjaHorbach Feb 22, 2024
2317ae5
Refactore code and add new utilit for sections
ZhenjaHorbach Feb 27, 2024
4ad85e6
Update branch
ZhenjaHorbach Feb 27, 2024
279cf4a
Update getSectionsWithIndexOffset
ZhenjaHorbach Feb 27, 2024
7eb76d6
Remove unnecessary code
ZhenjaHorbach Feb 27, 2024
6794d02
Update branch
ZhenjaHorbach Feb 28, 2024
fc784e6
Remove unnecessary code
ZhenjaHorbach Feb 28, 2024
8154f93
Update branch
ZhenjaHorbach Feb 29, 2024
5d440d7
Update branch
ZhenjaHorbach Feb 29, 2024
86ec95b
Update branch
ZhenjaHorbach Mar 4, 2024
0db97cf
Remove unnecessary code
ZhenjaHorbach Mar 4, 2024
1acf6b2
Fix comments
ZhenjaHorbach Mar 5, 2024
27576a0
Merge branch 'main' into item-in-report-list-is-not-highlighted-and-l…
ZhenjaHorbach Mar 5, 2024
7d67f31
Remove unnecessary code
ZhenjaHorbach Mar 5, 2024
fadf22a
Update branch
ZhenjaHorbach Mar 11, 2024
131435c
Update branch
ZhenjaHorbach Mar 15, 2024
826fbec
Update stories
ZhenjaHorbach Mar 15, 2024
672e271
Update branch
ZhenjaHorbach Mar 18, 2024
fba4343
Update branch and update perf-test
ZhenjaHorbach Mar 18, 2024
ff625d2
Fix lint issues
ZhenjaHorbach Mar 18, 2024
10fcd80
Update branch
ZhenjaHorbach Mar 19, 2024
713eee6
Update branch
ZhenjaHorbach Mar 20, 2024
90e3f46
Update branch and fix conflicts
ZhenjaHorbach Mar 25, 2024
83c4c31
Fix comments
ZhenjaHorbach Mar 25, 2024
e508f98
Update bransh and fix conflicts
ZhenjaHorbach Mar 28, 2024
98edd68
Update branch and fix conflicts
ZhenjaHorbach Mar 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function YearPickerModal({isVisible, years, currentYear = new Date().getFullYear
const yearsList = searchText === '' ? years : years.filter((year) => year.text.includes(searchText));
return {
headerMessage: !yearsList.length ? translate('common.noResultsFound') : '',
sections: [{data: yearsList.sort((a, b) => b.value - a.value), indexOffset: 0}],
sections: [{data: yearsList.sort((a, b) => b.value - a.value)}],
};
}, [years, searchText, translate]);

Expand Down
3 changes: 0 additions & 3 deletions src/components/MoneyRequestConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -378,14 +378,12 @@ function MoneyRequestConfirmationList(props) {
title: translate('moneyRequestConfirmationList.paidBy'),
data: [formattedPayeeOption],
shouldShow: true,
indexOffset: 0,
isDisabled: shouldDisablePaidBySection,
},
{
title: translate('moneyRequestConfirmationList.splitWith'),
data: formattedParticipantsList,
shouldShow: true,
indexOffset: 1,
},
);
} else {
Expand All @@ -397,7 +395,6 @@ function MoneyRequestConfirmationList(props) {
title: translate('common.to'),
data: formattedSelectedParticipants,
shouldShow: true,
indexOffset: 0,
});
}
return sections;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -422,14 +422,12 @@ function MoneyTemporaryForRefactorRequestConfirmationList({
title: translate('moneyRequestConfirmationList.paidBy'),
data: [formattedPayeeOption],
shouldShow: true,
indexOffset: 0,
isDisabled: shouldDisablePaidBySection,
},
{
title: translate('moneyRequestConfirmationList.splitWith'),
data: formattedParticipantsList,
shouldShow: true,
indexOffset: 1,
},
);
} else {
Expand All @@ -441,7 +439,6 @@ function MoneyTemporaryForRefactorRequestConfirmationList({
title: translate('common.to'),
data: formattedSelectedParticipants,
shouldShow: true,
indexOffset: 0,
});
}
return sections;
Expand Down
6 changes: 4 additions & 2 deletions src/components/OptionsList/BaseOptionsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import SectionList from '@components/SectionList';
import Text from '@components/Text';
import usePrevious from '@hooks/usePrevious';
import useThemeStyles from '@hooks/useThemeStyles';
import getSectionsWithIndexOffset from '@libs/getSectionsWithIndexOffset';
import type {OptionData} from '@libs/ReportUtils';
import StringUtils from '@libs/StringUtils';
import variables from '@styles/variables';
Expand Down Expand Up @@ -67,6 +68,7 @@ function BaseOptionsList(

const listContainerStyles = useMemo(() => listContainerStylesProp ?? [styles.flex1], [listContainerStylesProp, styles.flex1]);
const contentContainerStyles = useMemo(() => [safeAreaPaddingBottomStyle, contentContainerStylesProp], [contentContainerStylesProp, safeAreaPaddingBottomStyle]);
const sectionsWithIndexOffset = getSectionsWithIndexOffset(sections);

/**
* This helper function is used to memoize the computation needed for getItemLayout. It is run whenever section data changes.
Expand Down Expand Up @@ -182,7 +184,7 @@ function BaseOptionsList(
option={item}
showTitleTooltip={showTitleTooltip}
hoverStyle={optionHoveredStyle}
optionIsFocused={!disableFocusOptions && !isItemDisabled && focusedIndex === index + section.indexOffset}
optionIsFocused={!disableFocusOptions && !isItemDisabled && focusedIndex === index + (section.indexOffset ?? 0)}
onSelectRow={onSelectRow}
isSelected={isSelected}
showSelectedState={canSelectMultipleOptions}
Expand Down Expand Up @@ -248,7 +250,7 @@ function BaseOptionsList(
onScroll={onScroll}
contentContainerStyle={contentContainerStyles}
showsVerticalScrollIndicator={showScrollIndicator}
sections={sections}
sections={sectionsWithIndexOffset}
keyExtractor={extractKey}
stickySectionHeadersEnabled={false}
renderItem={renderItem}
Expand Down
2 changes: 1 addition & 1 deletion src/components/OptionsList/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type Section = {
title: string;

/** The initial index of this section given the total number of options in each section's data array */
indexOffset: number;
indexOffset?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we making this optional instead of removing it, or explicitly passing it everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


/** Array of options */
data: OptionData[];
Expand Down
3 changes: 0 additions & 3 deletions src/components/OptionsSelector/optionsSelectorPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ const propTypes = {
/** Title of the section */
title: PropTypes.string,

/** The initial index of this section given the total number of options in each section's data array */
indexOffset: PropTypes.number,

/** Array of options */
data: PropTypes.arrayOf(optionPropTypes),

Expand Down
4 changes: 3 additions & 1 deletion src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import useKeyboardShortcut from '@hooks/useKeyboardShortcut';
import useLocalize from '@hooks/useLocalize';
import usePrevious from '@hooks/usePrevious';
import useThemeStyles from '@hooks/useThemeStyles';
import getSectionsWithIndexOffset from '@libs/getSectionsWithIndexOffset';
import Log from '@libs/Log';
import variables from '@styles/variables';
import CONST from '@src/CONST';
Expand Down Expand Up @@ -78,6 +79,7 @@ function BaseSelectionList<TItem extends ListItem>(
const isFocused = useIsFocused();
const [maxToRenderPerBatch, setMaxToRenderPerBatch] = useState(shouldUseDynamicMaxToRenderPerBatch ? 0 : CONST.MAX_TO_RENDER_PER_BATCH.DEFAULT);
const [isInitialSectionListRender, setIsInitialSectionListRender] = useState(true);
const sectionsWithIndexOffset = getSectionsWithIndexOffset(sections);

/**
* Iterates through the sections and items inside each section, and builds 3 arrays along the way:
Expand Down Expand Up @@ -449,7 +451,7 @@ function BaseSelectionList<TItem extends ListItem>(
{!headerMessage && !canSelectMultiple && customListHeader}
<SectionList
ref={listRef}
sections={sections}
sections={sectionsWithIndexOffset}
stickySectionHeadersEnabled={false}
renderSectionHeader={renderSectionHeader}
renderItem={renderItem}
Expand Down
3 changes: 0 additions & 3 deletions src/components/SelectionList/selectionListPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,6 @@ const propTypes = {
/** Title of the section */
title: PropTypes.string,

/** The initial index of this section given the total number of options in each section's data array */
indexOffset: PropTypes.number,

/** Array of options */
data: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.shape(userListItemPropTypes.item), PropTypes.shape(radioListItemPropTypes.item)])),

Expand Down
2 changes: 1 addition & 1 deletion src/components/StatePicker/StateSelectorModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ function StateSelectorModal({currentState, isVisible, onClose = () => {}, onStat
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
textInputLabel={label || translate('common.state')}
textInputValue={searchValue}
sections={[{data: searchResults, indexOffset: 0}]}
sections={[{data: searchResults}]}
onSelectRow={onStateSelected}
onChangeText={setSearchValue}
initiallyFocusedOptionKey={currentState}
Expand Down
39 changes: 0 additions & 39 deletions src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ type PayeePersonalDetails = {
type CategorySectionBase = {
title: string | undefined;
shouldShow: boolean;
indexOffset: number;
};

type CategorySection = CategorySectionBase & {
Expand Down Expand Up @@ -154,7 +153,6 @@ type MemberForList = {

type SectionForSearchTerm = {
section: CategorySection;
newIndexOffset: number;
};

type GetOptions = {
Expand Down Expand Up @@ -965,14 +963,11 @@ function getCategoryListSections(
const categorySections: CategoryTreeSection[] = [];
const numberOfCategories = enabledCategories.length;

let indexOffset = 0;

if (numberOfCategories === 0 && selectedOptions.length > 0) {
categorySections.push({
// "Selected" section
title: '',
shouldShow: false,
indexOffset,
data: getCategoryOptionTree(selectedOptions, true),
});

Expand All @@ -986,7 +981,6 @@ function getCategoryListSections(
// "Search" section
title: '',
shouldShow: true,
indexOffset,
data: getCategoryOptionTree(searchCategories, true),
});

Expand All @@ -1002,7 +996,6 @@ function getCategoryListSections(
// "All" section when items amount less than the threshold
title: '',
shouldShow: false,
indexOffset,
data: getCategoryOptionTree(enabledAndSelectedCategories),
});

Expand All @@ -1014,11 +1007,8 @@ function getCategoryListSections(
// "Selected" section
title: '',
shouldShow: false,
indexOffset,
data: getCategoryOptionTree(selectedOptions, true),
});

indexOffset += selectedOptions.length;
}

const filteredRecentlyUsedCategories = recentlyUsedCategories
Expand All @@ -1035,11 +1025,8 @@ function getCategoryListSections(
// "Recent" section
title: Localize.translateLocal('common.recent'),
shouldShow: true,
indexOffset,
data: getCategoryOptionTree(cutRecentlyUsedCategories, true),
});

indexOffset += filteredRecentlyUsedCategories.length;
}

const filteredCategories = enabledCategories.filter((category) => !selectedOptionNames.includes(category.name));
Expand All @@ -1048,7 +1035,6 @@ function getCategoryListSections(
// "All" section when items amount more than the threshold
title: Localize.translateLocal('common.all'),
shouldShow: true,
indexOffset,
data: getCategoryOptionTree(filteredCategories),
});

Expand Down Expand Up @@ -1083,7 +1069,6 @@ function getTagListSections(tags: Tag[], recentlyUsedTags: string[], selectedOpt
const selectedOptionNames = selectedOptions.map((selectedOption) => selectedOption.name);
const enabledTags = [...selectedOptions, ...sortedTags.filter((tag) => tag.enabled && !selectedOptionNames.includes(tag.name))];
const numberOfTags = enabledTags.length;
let indexOffset = 0;

// If all tags are disabled but there's a previously selected tag, show only the selected tag
if (numberOfTags === 0 && selectedOptions.length > 0) {
Expand All @@ -1096,7 +1081,6 @@ function getTagListSections(tags: Tag[], recentlyUsedTags: string[], selectedOpt
// "Selected" section
title: '',
shouldShow: false,
indexOffset,
data: getTagsOptions(selectedTagOptions),
});

Expand All @@ -1110,7 +1094,6 @@ function getTagListSections(tags: Tag[], recentlyUsedTags: string[], selectedOpt
// "Search" section
title: '',
shouldShow: true,
indexOffset,
data: getTagsOptions(searchTags),
});

Expand All @@ -1122,7 +1105,6 @@ function getTagListSections(tags: Tag[], recentlyUsedTags: string[], selectedOpt
// "All" section when items amount less than the threshold
title: '',
shouldShow: false,
indexOffset,
data: getTagsOptions(enabledTags),
});

Expand All @@ -1148,11 +1130,8 @@ function getTagListSections(tags: Tag[], recentlyUsedTags: string[], selectedOpt
// "Selected" section
title: '',
shouldShow: true,
indexOffset,
data: getTagsOptions(selectedTagOptions),
});

indexOffset += selectedOptions.length;
}

if (filteredRecentlyUsedTags.length > 0) {
Expand All @@ -1162,18 +1141,14 @@ function getTagListSections(tags: Tag[], recentlyUsedTags: string[], selectedOpt
// "Recent" section
title: Localize.translateLocal('common.recent'),
shouldShow: true,
indexOffset,
data: getTagsOptions(cutRecentlyUsedTags),
});

indexOffset += filteredRecentlyUsedTags.length;
}

tagSections.push({
// "All" section when items amount more than the threshold
title: Localize.translateLocal('common.all'),
shouldShow: true,
indexOffset,
data: getTagsOptions(filteredTags),
});

Expand Down Expand Up @@ -1236,8 +1211,6 @@ function getTaxRatesSection(taxRates: TaxRatesWithDefault | undefined, selectedO
const enabledTaxRates = sortedTaxRates.filter((taxRate) => !taxRate.isDisabled);
const numberOfTaxRates = enabledTaxRates.length;

let indexOffset = 0;

// If all tax are disabled but there's a previously selected tag, show only the selected tag
if (numberOfTaxRates === 0 && selectedOptions.length > 0) {
const selectedTaxRateOptions = selectedOptions.map((option) => ({
Expand All @@ -1249,7 +1222,6 @@ function getTaxRatesSection(taxRates: TaxRatesWithDefault | undefined, selectedO
// "Selected" sectiong
title: '',
shouldShow: false,
indexOffset,
data: getTaxRatesOptions(selectedTaxRateOptions),
});

Expand All @@ -1263,7 +1235,6 @@ function getTaxRatesSection(taxRates: TaxRatesWithDefault | undefined, selectedO
// "Search" section
title: '',
shouldShow: true,
indexOffset,
data: getTaxRatesOptions(searchTaxRates),
});

Expand All @@ -1275,7 +1246,6 @@ function getTaxRatesSection(taxRates: TaxRatesWithDefault | undefined, selectedO
// "All" section when items amount less than the threshold
title: '',
shouldShow: false,
indexOffset,
data: getTaxRatesOptions(enabledTaxRates),
});

Expand All @@ -1299,18 +1269,14 @@ function getTaxRatesSection(taxRates: TaxRatesWithDefault | undefined, selectedO
// "Selected" section
title: '',
shouldShow: true,
indexOffset,
data: getTaxRatesOptions(selectedTaxRatesOptions),
});

indexOffset += selectedOptions.length;
}

policyRatesSections.push({
// "All" section when number of items are more than the threshold
title: '',
shouldShow: true,
indexOffset,
data: getTaxRatesOptions(filteredTaxRates),
});

Expand Down Expand Up @@ -1985,7 +1951,6 @@ function formatSectionsFromSearchTerm(
filteredRecentReports: ReportUtils.OptionData[],
filteredPersonalDetails: ReportUtils.OptionData[],
maxOptionsSelected: boolean,
indexOffset = 0,
personalDetails: OnyxEntry<PersonalDetailsList> = {},
shouldGetOptionDetails = false,
): SectionForSearchTerm {
Expand All @@ -2003,9 +1968,7 @@ function formatSectionsFromSearchTerm(
})
: selectedOptions,
shouldShow: selectedOptions.length > 0,
indexOffset,
},
newIndexOffset: indexOffset + selectedOptions.length,
};
}

Expand All @@ -2029,9 +1992,7 @@ function formatSectionsFromSearchTerm(
})
: selectedParticipantsWithoutDetails,
shouldShow: selectedParticipantsWithoutDetails.length > 0,
indexOffset,
},
newIndexOffset: indexOffset + selectedParticipantsWithoutDetails.length,
};
}

Expand Down
9 changes: 9 additions & 0 deletions src/libs/getSectionsWithIndexOffset.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type {SectionListData} from 'react-native';

/** Returns a list of sections with IndexOffset */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Returns a list of sections with IndexOffset */
/**
* Returns a list of sections with indexOffset
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

export default function getSectionsWithIndexOffset<ItemT, SectionT>(sections: Array<SectionListData<ItemT, SectionT>>) {
return sections.map((section, index) => {
const indexOffset = [...sections].splice(0, index).reduce((acc, curr) => acc + (curr.data?.length ?? 0), 0);
return {...section, indexOffset};
});
}
1 change: 1 addition & 0 deletions src/pages/EditReportFieldDropdownPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ function EditReportFieldDropdownPage({fieldName, onSubmit, fieldID, fieldValue,
textInputLabel={translate('common.search')}
boldStyle
sections={sections}
focusedIndex={0}
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this have to be set to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly the screen where the bug was found
This is needed for searching
So that during the search the first element is selected

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put that in a comment to make this very clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain the why in the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has already been fixed)

value={searchValue}
onSelectRow={(option: Record<string, string>) => onSubmit({[fieldID]: option.text})}
onChangeText={setSearchValue}
Expand Down
Loading
Loading