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.

Sign up for GitHub

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

Jump to bottom

[TS migration] Migrate 'ReportWelcomeText.js' component to TypeScript #33251

Merged
merged 8 commits into from
Jan 2, 2024
Merged

[TS migration] Migrate 'ReportWelcomeText.js' component to TypeScript #33251

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3f157a4
TS migration for ReportWelcomeText done
nikos-amofa Dec 18, 2023
9e7f974
use hooks
nikos-amofa Dec 18, 2023
3fad7ce
comments addressed
nikos-amofa Dec 19, 2023
e9e32fb
comment with issue number for ts-expect-error
nikos-amofa Dec 19, 2023
73b8b7f
comments addressed
nikos-amofa Dec 19, 2023
72a8d66
Merge branch 'main' of github-nikos:nikosamofa/expensify-app into ts-…
nikos-amofa Dec 20, 2023
ca2eab8
Merge branch 'main' of github-nikos:nikosamofa/expensify-app into ts-…
nikos-amofa Dec 21, 2023
e5cd846
conflict resolved
nikos-amofa Dec 28, 2023
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
114 changes: 40 additions & 74 deletions src/components/ReportWelcomeText.js → src/components/ReportWelcomeText.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,92 +1,65 @@
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import _ from 'lodash';
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to remove lodash completely in this file:

{!_.isEmpty(pronouns) && <Text>{` (${pronouns})`}</Text>}
{!!pronouns && <Text>{` (${pronouns})`}</Text>}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed by 3fad7ce

import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import {OnyxEntry, withOnyx} from 'react-native-onyx';
import useLocalize from '@hooks/useLocalize';
import useThemeStyles from '@hooks/useThemeStyles';
import compose from '@libs/compose';
import Navigation from '@libs/Navigation/Navigation';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import * as PolicyUtils from '@libs/PolicyUtils';
import * as ReportUtils from '@libs/ReportUtils';
import reportPropTypes from '@pages/reportPropTypes';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {PersonalDetailsList, Policy, Report} from '@src/types/onyx';
import Text from './Text';
import UserDetailsTooltip from './UserDetailsTooltip';
import withLocalize, {withLocalizePropTypes} from './withLocalize';

const personalDetailsPropTypes = PropTypes.shape({
/** The login of the person (either email or phone number) */
login: PropTypes.string,

/** The URL of the person's avatar (there should already be a default avatar if
the person doesn't have their own avatar uploaded yet, except for anon users) */
avatar: PropTypes.string,

/** This is either the user's full name, or their login if full name is an empty string */
displayName: PropTypes.string,
});

const propTypes = {
/** The report currently being looked at */
report: reportPropTypes,

/** The policy object for the current route */
policy: PropTypes.shape({
/** The name of the policy */
name: PropTypes.string,

/** The URL for the policy avatar */
avatar: PropTypes.string,
}),

/* Onyx Props */

type ReportWelcomeTextOnyxProps = {
/** All of the personal details for everyone */
personalDetails: PropTypes.objectOf(personalDetailsPropTypes),

...withLocalizePropTypes,
personalDetails: OnyxEntry<PersonalDetailsList>;
};

const defaultProps = {
report: {},
policy: {},
personalDetails: {},
type ReportWelcomeTextProps = ReportWelcomeTextOnyxProps & {
/** The report currently being looked at */
report: Report;

/** The policy for the current route */
policy: Policy;
};

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 ReportWelcomeTextProps = ReportWelcomeTextOnyxProps & {
/** The report currently being looked at */
report: Report;
/** The policy for the current route */
policy: Policy;
};
type ReportWelcomeTextProps = ReportWelcomeTextOnyxProps & {
/** The report currently being looked at */
report: OnyxEntry<Report>;
/** The policy for the current route */
policy: OnyxEntry<Policy>;
};

Since ReportActionItemCreated is passing Onyx entries to report and policy, let's type them as OnyxEntry as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed by 3fad7ce

function ReportWelcomeText(props) {
function ReportWelcomeText({report = {} as Report, policy = {} as Policy, personalDetails = {}}: ReportWelcomeTextProps) {
const {translate} = useLocalize();
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
function ReportWelcomeText({report = {} as Report, policy = {} as Policy, personalDetails = {}}: ReportWelcomeTextProps) {
function ReportWelcomeText({report, policy, personalDetails = {}}: ReportWelcomeTextProps) {

Let's avoid assigning those variables to empty objects {}. report and policy is being used in ReportActionItemCreated and these props comes from Onyx, so the value can be null and you can treat the nullability in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed by 3fad7ce

const styles = useThemeStyles();
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(props.report);
const isChatRoom = ReportUtils.isChatRoom(props.report);
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
const isChatRoom = ReportUtils.isChatRoom(report);
const isDefault = !(isChatRoom || isPolicyExpenseChat);
const participantAccountIDs = lodashGet(props.report, 'participantAccountIDs', []);
const participantAccountIDs = report?.participantAccountIDs ?? [];
const isMultipleParticipant = participantAccountIDs.length > 1;
const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(
OptionsListUtils.getPersonalDetailsForAccountIDs(participantAccountIDs, props.personalDetails),
// TODO: Remove type assertion (`as PersonalDetailsList`) after `src/libs/OptionsListUtils.js` is migrated into ts
OptionsListUtils.getPersonalDetailsForAccountIDs(participantAccountIDs, personalDetails) as PersonalDetailsList,
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is not aligned with the TS Guidelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabioh8010 I tried to find the issue for TS migration of OptionsListUtils, but was not able to find it, so commented TODO like above.
Could you kindly point out the issue number for it?
OR will it be okay to remove this TODO comment here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nikosamofa Here it is!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed by e9e32fb

isMultipleParticipant,
);
const isUserPolicyAdmin = PolicyUtils.isPolicyAdmin(props.policy);
const roomWelcomeMessage = ReportUtils.getRoomWelcomeMessage(props.report, isUserPolicyAdmin);
const moneyRequestOptions = ReportUtils.getMoneyRequestOptions(props.report, participantAccountIDs);
const isUserPolicyAdmin = PolicyUtils.isPolicyAdmin(policy);
const roomWelcomeMessage = ReportUtils.getRoomWelcomeMessage(report, isUserPolicyAdmin);
const moneyRequestOptions = ReportUtils.getMoneyRequestOptions(report, participantAccountIDs);

return (
<>
<View>
<Text style={[styles.textHero]}>
{isChatRoom ? props.translate('reportActionsView.welcomeToRoom', {roomName: ReportUtils.getReportName(props.report)}) : props.translate('reportActionsView.sayHello')}
{isChatRoom ? translate('reportActionsView.welcomeToRoom', {roomName: ReportUtils.getReportName(report)}) : translate('reportActionsView.sayHello')}
</Text>
</View>
<Text style={[styles.mt3, styles.mw100]}>
{isPolicyExpenseChat && (
<>
<Text>{props.translate('reportActionsView.beginningOfChatHistoryPolicyExpenseChatPartOne')}</Text>
<Text style={[styles.textStrong]}>{ReportUtils.getDisplayNameForParticipant(props.report.ownerAccountID)}</Text>
<Text>{props.translate('reportActionsView.beginningOfChatHistoryPolicyExpenseChatPartTwo')}</Text>
<Text style={[styles.textStrong]}>{ReportUtils.getPolicyName(props.report)}</Text>
<Text>{props.translate('reportActionsView.beginningOfChatHistoryPolicyExpenseChatPartThree')}</Text>
<Text>{translate('reportActionsView.beginningOfChatHistoryPolicyExpenseChatPartOne')}</Text>
<Text style={[styles.textStrong]}>{ReportUtils.getDisplayNameForParticipant(report.ownerAccountID)}</Text>
<Text>{translate('reportActionsView.beginningOfChatHistoryPolicyExpenseChatPartTwo')}</Text>
<Text style={[styles.textStrong]}>{ReportUtils.getPolicyName(report)}</Text>
<Text>{translate('reportActionsView.beginningOfChatHistoryPolicyExpenseChatPartThree')}</Text>
</>
)}
{isChatRoom && (
Expand All @@ -95,20 +68,20 @@ function ReportWelcomeText(props) {
{roomWelcomeMessage.showReportName && (
<Text
style={[styles.textStrong]}
onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(props.report.reportID))}
onPress={() => Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report.reportID))}
suppressHighlighting
>
{ReportUtils.getReportName(props.report)}
{ReportUtils.getReportName(report)}
</Text>
)}
{roomWelcomeMessage.phrase2 !== undefined && <Text>{roomWelcomeMessage.phrase2}</Text>}
</>
)}
{isDefault && (
<Text>
<Text>{props.translate('reportActionsView.beginningOfChatHistory')}</Text>
{_.map(displayNamesWithTooltips, ({displayName, pronouns, accountID}, index) => (
<Text key={`${displayName}${pronouns}${index}`}>
<Text>{translate('reportActionsView.beginningOfChatHistory')}</Text>
{displayNamesWithTooltips.map(({displayName, pronouns, accountID}, index) => (
<Text key={`${displayName}${pronouns}${accountID}`}>
<UserDetailsTooltip accountID={accountID}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if you have already noticed this, but I changed

<Text key={`${displayName}${pronouns}${index}`}>

to

<Text key={`${displayName}${pronouns}${accountID}`}>

ESlint complained Do not use Array index in keys react/no-array-index-key

I assume accountID will be unique inside this array, can you confirm it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nikosamofa Another alternative (if accountID can't be used) is to suppress the rule with // eslint-disable-next-line react/no-array-index-key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed by 73b8b7f

{ReportUtils.isOptimisticPersonalDetail(accountID) ? (
<Text style={[styles.textStrong]}>{displayName}</Text>
Expand All @@ -124,29 +97,22 @@ function ReportWelcomeText(props) {
</UserDetailsTooltip>
{!_.isEmpty(pronouns) && <Text>{` (${pronouns})`}</Text>}
{index === displayNamesWithTooltips.length - 1 && <Text>.</Text>}
{index === displayNamesWithTooltips.length - 2 && <Text>{` ${props.translate('common.and')} `}</Text>}
{index === displayNamesWithTooltips.length - 2 && <Text>{` ${translate('common.and')} `}</Text>}
{index < displayNamesWithTooltips.length - 2 && <Text>, </Text>}
</Text>
))}
</Text>
)}
{(moneyRequestOptions.includes(CONST.IOU.TYPE.SEND) || moneyRequestOptions.includes(CONST.IOU.TYPE.REQUEST)) && (
<Text>{props.translate('reportActionsView.usePlusButton')}</Text>
)}
{(moneyRequestOptions.includes(CONST.IOU.TYPE.SEND) || moneyRequestOptions.includes(CONST.IOU.TYPE.REQUEST)) && <Text>{translate('reportActionsView.usePlusButton')}</Text>}
</Text>
</>
);
}

ReportWelcomeText.defaultProps = defaultProps;
ReportWelcomeText.propTypes = propTypes;
ReportWelcomeText.displayName = 'ReportWelcomeText';

export default compose(
withLocalize,
withOnyx({
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
}),
)(ReportWelcomeText);
export default withOnyx<ReportWelcomeTextProps, ReportWelcomeTextOnyxProps>({
personalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
})(ReportWelcomeText);
Loading