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

[TS migration] Migrate SearchPage #34655

Merged
merged 35 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
5ef38a6
[TS migration] Migrate SearchPage
ruben-rebelo Jan 17, 2024
a1bf42a
[TS migration] Added missing key on searchPage OnyxValues
ruben-rebelo Jan 17, 2024
0311d75
[TS migration] SearchPage feedback
ruben-rebelo Jan 18, 2024
043d7a6
[TS migration] SearchPage code improvements
ruben-rebelo Jan 18, 2024
3160959
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Jan 30, 2024
4b868f6
[TS migration] Partially addressed comments
ruben-rebelo Jan 22, 2024
0b6deab
[TS migration] SearchPage minor ts error fix
ruben-rebelo Jan 22, 2024
4885b59
[TS migration] SearchPage Lint
ruben-rebelo Jan 22, 2024
000eb11
[TS Migration] SearchPage route props improvement
ruben-rebelo Jan 24, 2024
221bed7
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Jan 29, 2024
be14da6
[TS migration][SearchPage] Code improvements after merged main
ruben-rebelo Jan 29, 2024
751c974
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Jan 29, 2024
db8c05f
[TS migration] Searchpage navigation prop
ruben-rebelo Jan 30, 2024
0b0ba7a
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Feb 2, 2024
33a370f
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Feb 7, 2024
5802e70
[TS migration][SearchPage] Migrate SearchPage after refactor
ruben-rebelo Feb 8, 2024
fe38639
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Feb 8, 2024
8ef09aa
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Feb 8, 2024
15ba310
[TS migration][SearchPage] TS fix
ruben-rebelo Feb 8, 2024
1532321
[TS migration][SearchPage] Typescript adjustements
ruben-rebelo Feb 8, 2024
2cd3918
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Feb 16, 2024
dbaabaf
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Feb 20, 2024
efa4b99
[TS migration][SearchPage] Updated types based on main
ruben-rebelo Feb 20, 2024
4430458
[TS migration][SearchPage] Minor ts issue
ruben-rebelo Feb 20, 2024
5c88c77
Revert "[TS migration][SearchPage] Minor ts issue"
ruben-rebelo Feb 27, 2024
333700b
Revert "[TS migration][SearchPage] Updated types based on main"
ruben-rebelo Feb 27, 2024
75f3770
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Feb 27, 2024
2b01f41
[TS migration][SearchPage] TS issues fix
ruben-rebelo Feb 27, 2024
74061d7
[TS migration][SearchPage] Feedback
ruben-rebelo Mar 4, 2024
dcec860
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Mar 4, 2024
44ddd40
[TS migration][SearchPage] TS issues fix
ruben-rebelo Mar 4, 2024
7ba0847
[TS migration][SearchPage] Update src/components/SelectionList/types.ts
ruben-rebelo Mar 5, 2024
f6e6552
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo Mar 5, 2024
57fcb34
Merge branch 'ts-migration/searchPage' of https://github.com/ruben-re…
ruben-rebelo Mar 5, 2024
bef7c71
[TS migration][SeachPage] Lint
ruben-rebelo Mar 5, 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 @@ -31,7 +31,7 @@ function YearPickerModal({isVisible, years, currentYear = new Date().getFullYear
const {translate} = useLocalize();
const [searchText, setSearchText] = useState('');
const {sections, headerMessage} = useMemo(() => {
const yearsList = searchText === '' ? years : years.filter((year) => year.text.includes(searchText));
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}],
Expand Down
6 changes: 3 additions & 3 deletions src/components/SelectionList/BaseListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,19 @@ function BaseListItem<TItem extends ListItem>({
{...bind}
onPress={() => onSelectRow(item)}
disabled={isDisabled}
accessibilityLabel={item.text}
accessibilityLabel={item.text ?? ''}
role={CONST.ROLE.BUTTON}
hoverDimmingValue={1}
hoverStyle={!item.isSelected && styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldPreventDefaultFocusOnSelectRow ? (e) => e.preventDefault() : undefined}
nativeID={keyForList}
nativeID={keyForList ?? ''}
style={pressableStyle}
>
<View style={wrapperStyle}>
{canSelectMultiple && (
<PressableWithFeedback
accessibilityLabel={item.text}
accessibilityLabel={item.text ?? ''}
role={CONST.ROLE.BUTTON}
disabled={isDisabled}
onPress={handleCheckboxPress}
Expand Down
4 changes: 2 additions & 2 deletions src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ function BaseSelectionList<TItem extends ListItem>(
onDismissError={() => onDismissError?.(item)}
shouldPreventDefaultFocusOnSelectRow={shouldPreventDefaultFocusOnSelectRow}
rightHandSideComponent={rightHandSideComponent}
keyForList={item.keyForList}
keyForList={item.keyForList ?? ''}
isMultilineSupported={isRowMultilineSupported}
/>
);
Expand Down Expand Up @@ -457,7 +457,7 @@ function BaseSelectionList<TItem extends ListItem>(
getItemLayout={getItemLayout}
onScroll={onScroll}
onScrollBeginDrag={onScrollBeginDrag}
keyExtractor={(item) => item.keyForList}
keyExtractor={(item, index) => item.keyForList ?? `${index}`}
extraData={focusedIndex}
indicatorStyle="white"
keyboardShouldPersistTaps="always"
Expand Down
2 changes: 1 addition & 1 deletion src/components/SelectionList/RadioListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ function RadioListItem({
<View style={[styles.flex1, styles.alignItemsStart]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.text}
text={item.text ?? ''}
style={[
styles.optionDisplayName,
isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
Expand Down
2 changes: 1 addition & 1 deletion src/components/SelectionList/TableListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function TableListItem({
<View style={[styles.flex1, styles.flexColumn, styles.justifyContentCenter, styles.alignItemsStretch]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.text}
text={item.text ?? ''}
style={[
styles.optionDisplayName,
isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
Expand Down
4 changes: 2 additions & 2 deletions src/components/SelectionList/UserListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ function UserListItem({
}
keyForList={item.keyForList}
>
{(hovered) => (
{(hovered?: boolean) => (
<>
{!!item.icons &&
(item.shouldShowSubscript ? (
Expand All @@ -81,7 +81,7 @@ function UserListItem({
<View style={[styles.flex1, styles.flexColumn, styles.justifyContentCenter, styles.alignItemsStretch, styles.optionRow]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.text}
text={item.text ?? ''}
style={[
styles.optionDisplayName,
isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText,
Expand Down
22 changes: 15 additions & 7 deletions src/components/SelectionList/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import type {ReactElement, ReactNode} from 'react';
import type {GestureResponderEvent, InputModeOptions, LayoutChangeEvent, SectionListData, StyleProp, TextStyle, ViewStyle} from 'react-native';
import type {MaybePhraseKey} from '@libs/Localize';
import type CONST from '@src/CONST';
import type {Errors, Icon, PendingAction} from '@src/types/onyx/OnyxCommon';
import type {ReceiptErrors} from '@src/types/onyx/Transaction';
import type ChildrenProps from '@src/types/utils/ChildrenProps';
Expand All @@ -12,7 +14,7 @@ type CommonListItemProps<TItem> = {
isFocused?: boolean;

/** Whether this item is disabled */
isDisabled?: boolean;
isDisabled?: boolean | null;

/** Whether this item should show Tooltip */
showTooltip: boolean;
Expand Down Expand Up @@ -47,19 +49,19 @@ type CommonListItemProps<TItem> = {

type ListItem = {
/** Text to display */
text: string;
text?: string;

/** Alternate text to display */
alternateText?: string | null;

/** Key used internally by React */
keyForList: string;
keyForList?: string | null;

/** Whether this option is selected */
isSelected?: boolean;

/** Whether this option is disabled for selection */
isDisabled?: boolean;
isDisabled?: boolean | null;
Copy link
Contributor

@ntdiary ntdiary Feb 19, 2024

Choose a reason for hiding this comment

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

Hi, my vacation has just ended. I can review this PR again tomorrow.
And curious, should we support null or delete the null type in Reportutils.OptionData? I personally prefer the latter, because the style guide says to Strive to type as strictly as possible, but I'm also open to get more inputs. :)

image

cc @ruben-rebelo, @fabioh8010 , @VickyStash

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if null is really needed in ReportUtils.OptionData we should support, otherwise let's delete it so we don't need to cascade this ?: boolean | null; up to this component! cc @ruben-rebelo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I've reverted all the changes made on SelectionList and updated SearchPage.
Please have a look @ntdiary

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if null is really needed in ReportUtils.OptionData we should support, otherwise let's delete it so we don't need to cascade this ?: boolean | null; up to this component! cc @ruben-rebelo

Yeah, I think you're right. In addition, I'm not sure if using as to cast the type is recommended here. It seems that there are no similar cases in the existing migration. 🤔

if (recentReports?.length > 0) {
    newSections.push({
        data: recentReports as ListItem[],
...

If removing the null of OptionData is difficult (e.g., big impact range, difficult to test, etc.), then we still have to support null in ListItem first. Type optimization may need to be done in the future.

BTW, maybe we can use MaybePhraseKey for offlineMessage, and change the textInputHint's type to MaybePhraseKey

Copy link
Contributor Author

@ruben-rebelo ruben-rebelo Feb 22, 2024

Choose a reason for hiding this comment

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

@ntdiary I'm afraid with this changed we can't avoid the assertion usage.
Do you prefer to revert this one and keep the nulls for this PR?
I'll update the offlineMessage and textInputHint with these suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ntdiary I'm afraid with this changed we can't avoid the assertion usage.
Do you prefer to revert this one and keep the nulls for this PR?

@ruben-rebelo, thank you very much for your attempt, then let's revert to the previous revision and let ListItem support the null type first. ❤️


/** List title is bold by default. Use this props to customize it */
isBold?: boolean;
Expand Down Expand Up @@ -90,6 +92,9 @@ type ListItem = {
/** Represents the index of the option within the section it came from */
index?: number;

/** ID of the report */
reportID?: string;

/** Whether this option should show subscript */
shouldShowSubscript?: boolean | null;

Expand Down Expand Up @@ -117,7 +122,7 @@ type ListItemProps = CommonListItemProps<ListItem> & {
type BaseListItemProps<TItem extends ListItem> = CommonListItemProps<TItem> & {
item: TItem;
shouldPreventDefaultFocusOnSelectRow?: boolean;
keyForList?: string;
keyForList?: string | null;
errors?: Errors | ReceiptErrors | null;
pendingAction?: PendingAction | null;
FooterComponent?: ReactElement;
Expand Down Expand Up @@ -158,7 +163,7 @@ type Section<TItem extends ListItem> = {

type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
/** Sections for the section list */
sections: Array<SectionListData<TItem, Section<TItem>>>;
sections: Array<SectionListData<TItem, Section<TItem>>> | typeof CONST.EMPTY_ARRAY;

/** Default renderer for every item in the list */
ListItem: typeof RadioListItem | typeof UserListItem | typeof TableListItem;
Expand All @@ -185,7 +190,7 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
textInputPlaceholder?: string;

/** Hint for the text input */
textInputHint?: string;
textInputHint?: MaybePhraseKey;

/** Value for the text input */
textInputValue?: string;
Expand Down Expand Up @@ -274,6 +279,9 @@ type BaseSelectionListProps<TItem extends ListItem> = Partial<ChildrenProps> & {
/** Styles for the list header wrapper */
listHeaderWrapperStyle?: StyleProp<ViewStyle>;

/** Whether to auto focus the Search Input */
autoFocus?: boolean;

/** Whether to wrap long text up to 2 lines */
isRowMultilineSupported?: boolean;
};
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ type AuthScreensParamList = SharedScreensParamList & {
[SCREENS.DESKTOP_SIGN_IN_REDIRECT]: undefined;
};

type RootStackParamList = PublicScreensParamList & AuthScreensParamList;
type RootStackParamList = PublicScreensParamList & AuthScreensParamList & SearchNavigatorParamList;

type BottomTabName = keyof BottomTabNavigatorParamList;

Expand Down
2 changes: 1 addition & 1 deletion src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1728,7 +1728,7 @@ function getOptions(
/**
* Build the options for the Search view
*/
function getSearchOptions(reports: Record<string, Report>, personalDetails: OnyxEntry<PersonalDetailsList>, searchValue = '', betas: Beta[] = []): GetOptions {
function getSearchOptions(reports: OnyxCollection<Report>, personalDetails: OnyxEntry<PersonalDetailsList>, searchValue = '', betas: Beta[] = []): GetOptions {
Timing.start(CONST.TIMING.LOAD_SEARCH_OPTIONS);
Performance.markStart(CONST.TIMING.LOAD_SEARCH_OPTIONS);
const options = getOptions(reports, personalDetails, {
Expand Down
2 changes: 1 addition & 1 deletion src/pages/RoomMembersPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ function RoomMembersPage({report, session, policies}: RoomMembersPageProps) {
});
});

result = result.sort((value1, value2) => localeCompare(value1.text, value2.text));
result = result.sort((value1, value2) => localeCompare(value1.text ?? '', value2.text ?? ''));

return result;
};
Expand Down
71 changes: 33 additions & 38 deletions src/pages/SearchPage/index.js → src/pages/SearchPage/index.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import PropTypes from 'prop-types';
import type {StackScreenProps} from '@react-navigation/stack';
import React, {useEffect, useMemo, useState} from 'react';
import {View} from 'react-native';
import type {OnyxCollection, OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import {usePersonalDetails} from '@components/OnyxProvider';
import ScreenWrapper from '@components/ScreenWrapper';
Expand All @@ -12,59 +12,56 @@ import useDebouncedState from '@hooks/useDebouncedState';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useThemeStyles from '@hooks/useThemeStyles';
import type {MaybePhraseKey} from '@libs/Localize';
import Navigation from '@libs/Navigation/Navigation';
import type {RootStackParamList} from '@libs/Navigation/types';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import Performance from '@libs/Performance';
import * as ReportUtils from '@libs/ReportUtils';
import reportPropTypes from '@pages/reportPropTypes';
import * as Report from '@userActions/Report';
import Timing from '@userActions/Timing';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type SCREENS from '@src/SCREENS';
import type * as OnyxTypes from '@src/types/onyx';
import SearchPageFooter from './SearchPageFooter';

const propTypes = {
/* Onyx Props */

type SearchPageOnyxProps = {
/** Beta features list */
betas: PropTypes.arrayOf(PropTypes.string),
betas: OnyxEntry<OnyxTypes.Beta[]>;

/** All reports shared with the user */
reports: PropTypes.objectOf(reportPropTypes),
reports: OnyxCollection<OnyxTypes.Report>;

/** Whether or not we are searching for reports on the server */
isSearchingForReports: PropTypes.bool,

/**
* The navigation prop passed by the navigator.
*
* This is required because transitionEnd event doesn't trigger in the automated testing environment.
*/
navigation: PropTypes.shape({}),
isSearchingForReports: OnyxEntry<boolean>;
};

const defaultProps = {
betas: [],
reports: {},
isSearchingForReports: false,
navigation: {},
type SearchPageProps = SearchPageOnyxProps & StackScreenProps<RootStackParamList, typeof SCREENS.SEARCH_ROOT>;

type SearchPageSectionItem = {
data: ReportUtils.OptionData[];
shouldShow: boolean;
indexOffset: number;
};

type SearchPageSectionList = SearchPageSectionItem[];

const setPerformanceTimersEnd = () => {
Timing.end(CONST.TIMING.SEARCH_RENDER);
Performance.markEnd(CONST.TIMING.SEARCH_RENDER);
};

const SearchPageFooterInstance = <SearchPageFooter />;

function SearchPage({betas, reports, isSearchingForReports, navigation}) {
function SearchPage({betas, reports, isSearchingForReports, navigation}: SearchPageProps) {
const [isScreenTransitionEnd, setIsScreenTransitionEnd] = useState(false);
const {translate} = useLocalize();
const {isOffline} = useNetwork();
const themeStyles = useThemeStyles();
const personalDetails = usePersonalDetails();

const offlineMessage = isOffline ? [`${translate('common.youAppearToBeOffline')} ${translate('search.resultsAreLimited')}`, {isTranslated: true}] : '';
const offlineMessage: MaybePhraseKey = isOffline ? [`${translate('common.youAppearToBeOffline')} ${translate('search.resultsAreLimited')}`, {isTranslated: true}] : '';

const [searchValue, debouncedSearchValue, setSearchValue] = useDebouncedState('');

Expand All @@ -85,24 +82,24 @@ function SearchPage({betas, reports, isSearchingForReports, navigation}) {
} = useMemo(() => {
if (!isScreenTransitionEnd) {
return {
recentReports: {},
personalDetails: {},
userToInvite: {},
recentReports: [],
personalDetails: [],
userToInvite: null,
headerMessage: '',
};
}
const options = OptionsListUtils.getSearchOptions(reports, personalDetails, debouncedSearchValue.trim(), betas);
const options = OptionsListUtils.getSearchOptions(reports, personalDetails, debouncedSearchValue.trim(), betas ?? []);
const header = OptionsListUtils.getHeaderMessage(options.recentReports.length + options.personalDetails.length !== 0, Boolean(options.userToInvite), debouncedSearchValue);
return {...options, headerMessage: header};
}, [debouncedSearchValue, reports, personalDetails, betas, isScreenTransitionEnd]);

const sections = useMemo(() => {
const newSections = [];
const sections = useMemo((): SearchPageSectionList => {
const newSections: SearchPageSectionList = [];
let indexOffset = 0;

if (recentReports.length > 0) {
if (recentReports?.length > 0) {
newSections.push({
data: _.map(recentReports, (report) => ({...report, isBold: report.isUnread})),
data: recentReports.map((report) => ({...report, isBold: report.isUnread})),
shouldShow: true,
indexOffset,
});
Expand All @@ -129,7 +126,7 @@ function SearchPage({betas, reports, isSearchingForReports, navigation}) {
return newSections;
}, [localPersonalDetails, recentReports, userToInvite]);

const selectReport = (option) => {
const selectReport = (option: ReportUtils.OptionData) => {
if (!option) {
return;
}
Expand All @@ -138,7 +135,7 @@ function SearchPage({betas, reports, isSearchingForReports, navigation}) {
setSearchValue('');
Navigation.dismissModal(option.reportID);
} else {
Report.navigateToAndOpenReport([option.login]);
Report.navigateToAndOpenReport(option.login ? [option.login] : []);
}
};

Expand All @@ -163,7 +160,7 @@ function SearchPage({betas, reports, isSearchingForReports, navigation}) {
onBackButtonPress={Navigation.goBack}
/>
<View style={[themeStyles.flex1, themeStyles.w100, safeAreaPaddingBottomStyle]}>
<SelectionList
<SelectionList<ReportUtils.OptionData>
sections={didScreenTransitionEnd && isOptionsDataReady ? sections : CONST.EMPTY_ARRAY}
ListItem={UserListItem}
textInputValue={searchValue}
Expand All @@ -176,7 +173,7 @@ function SearchPage({betas, reports, isSearchingForReports, navigation}) {
onSelectRow={selectReport}
showLoadingPlaceholder={!didScreenTransitionEnd || !isOptionsDataReady}
footerContent={SearchPageFooterInstance}
isLoadingNewOptions={isSearchingForReports}
isLoadingNewOptions={isSearchingForReports ?? undefined}
/>
</View>
</>
Expand All @@ -185,11 +182,9 @@ function SearchPage({betas, reports, isSearchingForReports, navigation}) {
);
}

SearchPage.propTypes = propTypes;
SearchPage.defaultProps = defaultProps;
SearchPage.displayName = 'SearchPage';

export default withOnyx({
export default withOnyx<SearchPageProps, SearchPageOnyxProps>({
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
},
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/WorkspaceMembersPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ function WorkspaceMembersPage({policyMembers, personalDetails, route, policy, se
});
});

result = result.sort((a, b) => a.text.toLowerCase().localeCompare(b.text.toLowerCase()));
result = result.sort((a, b) => (a.text ?? '').toLowerCase().localeCompare((b.text ?? '').toLowerCase()));

return result;
};
Expand Down
Loading