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 21 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
4 changes: 2 additions & 2 deletions src/components/ScreenWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import useTackInputFocus from '@hooks/useTackInputFocus';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import * as Browser from '@libs/Browser';
import type {RootStackParamList} from '@libs/Navigation/types';
import type {RootStackParamList, SearchNavigatorParamList} from '@libs/Navigation/types';
Copy link
Contributor

@ntdiary ntdiary Feb 28, 2024

Choose a reason for hiding this comment

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

Suggested change
import type {RootStackParamList, SearchNavigatorParamList} from '@libs/Navigation/types';
import type {RootStackParamList} from '@libs/Navigation/types';

SearchPage doesn't pass navigation to ScreenWrapper, so we can revert this change.

import toggleTestToolsModal from '@userActions/TestTool';
import CONST from '@src/CONST';
import CustomDevMenu from './CustomDevMenu';
Expand Down Expand Up @@ -89,7 +89,7 @@ type ScreenWrapperProps = {
*
* This is required because transitionEnd event doesn't trigger in the testing environment.
*/
navigation?: StackNavigationProp<RootStackParamList>;
navigation?: StackNavigationProp<RootStackParamList> | StackNavigationProp<SearchNavigatorParamList>;
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
navigation?: StackNavigationProp<RootStackParamList> | StackNavigationProp<SearchNavigatorParamList>;
navigation?: StackNavigationProp<RootStackParamList>;


/** Whether to show offline indicator on wide screens */
shouldShowOfflineIndicatorInWideScreen?: boolean;
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 @@ -55,13 +55,13 @@ function BaseListItem<TItem extends User | RadioItem>({
<PressableWithFeedback
onPress={() => onSelectRow(item)}
disabled={isDisabled}
accessibilityLabel={item.text}
accessibilityLabel={item.text ?? ''}
role={CONST.ROLE.BUTTON}
hoverDimmingValue={1}
hoverStyle={styles.hoveredComponentBG}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
onMouseDown={shouldPreventDefaultFocusOnSelectRow ? (e) => e.preventDefault() : undefined}
nativeID={keyForList}
nativeID={keyForList ?? ''}
>
<View
style={[
Expand Down Expand Up @@ -130,7 +130,7 @@ function BaseListItem<TItem extends User | RadioItem>({
)}
{rightHandSideComponentRender()}
</View>
{isUserItem && item.invitedSecondaryLogin && (
{isUserItem && 'invitedSecondaryLogin' in item && item.invitedSecondaryLogin && (
<Text style={[styles.ml9, styles.ph5, styles.pb3, styles.textLabelSupporting]}>
{translate('workspace.people.invitedBySecondaryLogin', {secondaryLogin: item.invitedSecondaryLogin})}
</Text>
Expand Down
2 changes: 1 addition & 1 deletion src/components/SelectionList/BaseSelectionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@

const onSectionListLayout = useCallback(
(nativeEvent: LayoutChangeEvent) => {
onLayout?.(nativeEvent);

Check failure on line 317 in src/components/SelectionList/BaseSelectionList.tsx

View workflow job for this annotation

GitHub Actions / typecheck

Expected 0 arguments, but got 1.
scrollToFocusedIndexOnFirstRender(nativeEvent);
},
[onLayout, scrollToFocusedIndexOnFirstRender],
Expand Down Expand Up @@ -457,7 +457,7 @@
getItemLayout={getItemLayout}
onScroll={onScroll}
onScrollBeginDrag={onScrollBeginDrag}
keyExtractor={(item) => item.keyForList}
keyExtractor={(item) => item.keyForList ?? ''}
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
keyExtractor={(item) => item.keyForList ?? ''}
keyExtractor={(item, index) => item.keyForList ?? `${index}`}

How about falling back to using the index, like React does?

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 @@ -11,7 +11,7 @@ function RadioListItem({item, showTooltip, textStyles, alternateTextStyles}: Rad
<View style={[styles.flex1, styles.alignItemsStart]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.text}
text={item.text ?? ''}
textStyles={textStyles}
/>

Expand Down
2 changes: 1 addition & 1 deletion src/components/SelectionList/UserListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function UserListItem({item, textStyles, alternateTextStyles, showTooltip, style
<View style={[styles.flex1, styles.flexColumn, styles.justifyContentCenter, styles.alignItemsStretch, styles.optionRow]}>
<TextWithTooltip
shouldShowTooltip={showTooltip}
text={item.text}
text={item.text ?? ''}
textStyles={[textStyles, style]}
/>
{!!item.alternateText && (
Expand Down
39 changes: 23 additions & 16 deletions src/components/SelectionList/types.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type {ReactElement, ReactNode} from 'react';
import type {GestureResponderEvent, InputModeOptions, LayoutChangeEvent, SectionListData, StyleProp, TextStyle, ViewStyle} from 'react-native';
import type {GestureResponderEvent, InputModeOptions, SectionListData, StyleProp, TextStyle, ViewStyle} from 'react-native';
import type CONST from '@src/CONST';
import type {Errors, Icon, PendingAction} from '@src/types/onyx/OnyxCommon';
import type ChildrenProps from '@src/types/utils/ChildrenProps';

Expand All @@ -14,7 +15,7 @@ type CommonListItemProps<TItem> = {
alternateTextStyles?: StyleProp<TextStyle>;

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

/** Whether this item should show Tooltip */
showTooltip: boolean;
Expand All @@ -34,25 +35,25 @@ type CommonListItemProps<TItem> = {

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

/** Alternate text to display */
alternateText?: string;
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. ❤️


/** User accountID */
accountID?: number;
accountID?: number | null;

/** User login */
login?: string;
login?: string | null;

/** Element to show on the right side of the item */
rightElement?: ReactNode;
Expand All @@ -73,6 +74,9 @@ type User = {

/** Represents the index of the option within the section it came from */
index?: number;

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

type UserListItemProps = CommonListItemProps<User> & {
Expand All @@ -85,19 +89,19 @@ type UserListItemProps = CommonListItemProps<User> & {

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

/** Alternate text to display */
alternateText?: string;
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;

/** Represents the index of the section it came from */
sectionIndex?: number;
Expand All @@ -114,7 +118,7 @@ type RadioListItemProps = CommonListItemProps<RadioItem> & {
type BaseListItemProps<TItem extends User | RadioItem> = CommonListItemProps<TItem> & {
item: TItem;
shouldPreventDefaultFocusOnSelectRow?: boolean;
keyForList?: string;
keyForList?: string | null;
};

type Section<TItem extends User | RadioItem> = {
Expand All @@ -136,7 +140,7 @@ type Section<TItem extends User | RadioItem> = {

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

/** Whether this is a multi-select list */
canSelectMultiple?: boolean;
Expand Down Expand Up @@ -237,8 +241,11 @@ type BaseSelectionListProps<TItem extends User | RadioItem> = Partial<ChildrenPr
/** Whether to show the loading indicator for new options */
isLoadingNewOptions?: boolean;

/** Fired when the list is displayed with the items */
onLayout?: (event: LayoutChangeEvent) => void;
/** Custom callback when Selection List layout changes */
onLayout?: () => void;

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

type ItemLayout = {
Expand Down
2 changes: 1 addition & 1 deletion src/libs/OptionsListUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1704,7 +1704,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
59 changes: 31 additions & 28 deletions src/pages/SearchPage/index.js → src/pages/SearchPage/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
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 HeaderWithBackButton from '@components/HeaderWithBackButton';
import {usePersonalDetails} from '@components/OnyxProvider';
Expand All @@ -11,43 +12,47 @@
import useNetwork from '@hooks/useNetwork';
import useThemeStyles from '@hooks/useThemeStyles';
import Navigation from '@libs/Navigation/Navigation';
import type {SearchNavigatorParamList} 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,
isSearchingForReports: OnyxEntry<boolean>;
};

const defaultProps = {
betas: [],
reports: {},
isSearchingForReports: false,
type SearchPageProps = SearchPageOnyxProps & StackScreenProps<SearchNavigatorParamList, typeof SCREENS.SEARCH_ROOT>;
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
type SearchPageProps = SearchPageOnyxProps & StackScreenProps<SearchNavigatorParamList, typeof SCREENS.SEARCH_ROOT>;
type SearchPageProps = SearchPageOnyxProps;


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}) {
function SearchPage({betas, reports, isSearchingForReports}: SearchPageProps) {
const [isScreenTransitionEnd, setIsScreenTransitionEnd] = useState(false);
const {translate} = useLocalize();
const {isOffline} = useNetwork();
Expand Down Expand Up @@ -75,28 +80,28 @@
} = 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: recentReports,
shouldShow: true,
indexOffset,
});
indexOffset += recentReports.length;
indexOffset += recentReports?.length;
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
indexOffset += recentReports?.length;
indexOffset += recentReports.length;

recentReports.length > 0 is true. :)

}

if (localPersonalDetails.length > 0) {
Expand All @@ -119,7 +124,7 @@
return newSections;
}, [localPersonalDetails, recentReports, userToInvite]);

const selectReport = (option) => {
const selectReport = (option: ReportUtils.OptionData) => {
if (!option) {
return;
}
Expand All @@ -128,7 +133,7 @@
setSearchValue('');
Navigation.dismissModal(option.reportID);
} else {
Report.navigateToAndOpenReport([option.login]);
Report.navigateToAndOpenReport(option.login ? [option.login] : []);
}
};

Expand All @@ -151,11 +156,11 @@
onBackButtonPress={Navigation.goBack}
/>
<View style={[themeStyles.flex1, themeStyles.w100, safeAreaPaddingBottomStyle]}>
<SelectionList
<SelectionList<ReportUtils.OptionData>
sections={didScreenTransitionEnd && isOptionsDataReady ? sections : CONST.EMPTY_ARRAY}
textInputValue={searchValue}
textInputLabel={translate('optionsSelector.nameEmailOrPhoneNumber')}
textInputHint={offlineMessage}

Check failure on line 163 in src/pages/SearchPage/index.tsx

View workflow job for this annotation

GitHub Actions / typecheck

Type 'string | (string | { isTranslated: boolean; })[]' is not assignable to type 'string | undefined'.
onChangeText={setSearchValue}
headerMessage={headerMessage}
onLayout={setPerformanceTimersEnd}
Expand All @@ -163,7 +168,7 @@
onSelectRow={selectReport}
showLoadingPlaceholder={!didScreenTransitionEnd || !isOptionsDataReady}
footerContent={SearchPageFooterInstance}
isLoadingNewOptions={isSearchingForReports}
isLoadingNewOptions={isSearchingForReports ?? undefined}
/>
</View>
</>
Expand All @@ -172,11 +177,9 @@
);
}

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

export default withOnyx({
export default withOnyx<SearchPageProps, SearchPageOnyxProps>({
reports: {
key: ONYXKEYS.COLLECTION.REPORT,
},
Expand Down
Loading