-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 'ReportWelcomeText.js' component to TypeScript #33251
[TS migration] Migrate 'ReportWelcomeText.js' component to TypeScript #33251
Conversation
Should this go through a C+ before my review? |
Hm yeah that's what I thought too, I wonder why it didn't autoassign. |
@dangrous I think @nikosamofa was in a hurry and created the PR before being assigned to the issue by Expensify engineer after the C+ recommendation. That's why I wasn't assigned automatically. Could you please assign me here? |
src/components/ReportWelcomeText.tsx
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikosamofa Here it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed by e9e32fb
src/components/ReportWelcomeText.tsx
Outdated
type ReportWelcomeTextProps = ReportWelcomeTextOnyxProps & { | ||
/** The report currently being looked at */ | ||
report: Report; | ||
|
||
/** The policy for the current route */ | ||
policy: Policy; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed by 3fad7ce
src/components/ReportWelcomeText.tsx
Outdated
}; | ||
|
||
function ReportWelcomeText(props) { | ||
function ReportWelcomeText({report = {} as Report, policy = {} as Policy, personalDetails = {}}: ReportWelcomeTextProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed by 3fad7ce
src/components/ReportWelcomeText.tsx
Outdated
@@ -1,92 +1,65 @@ | |||
import lodashGet from 'lodash/get'; | |||
import PropTypes from 'prop-types'; | |||
import _ from 'lodash'; |
There was a problem hiding this comment.
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>}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, addressed by 3fad7ce
src/components/ReportWelcomeText.tsx
Outdated
<Text key={`${displayName}${pronouns}${index}`}> | ||
<Text>{translate('reportActionsView.beginningOfChatHistory')}</Text> | ||
{displayNamesWithTooltips.map(({displayName, pronouns, accountID}, index) => ( | ||
<Text key={`${displayName}${pronouns}${accountID}`}> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by 73b8b7f
src/components/ReportWelcomeText.tsx
Outdated
</> | ||
)} | ||
{isChatRoom && ( | ||
<> | ||
<Text>{roomWelcomeMessage.phrase1}</Text> | ||
{roomWelcomeMessage.showReportName && ( | ||
{roomWelcomeMessage.showReportName && !!report?.reportID && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROUTES.REPORT_WITH_ID_DETAILS.getRoute
requires string, not undefined type, so !!report?.reportID
is added here for the conditional rendering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikosamofa I think is better to do this condition inside onPress
callback instead of here, e.g.:
onPress={() => {
if (!report?.reportID) {
return;
}
Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report.reportID));
}}
You can also extract this onPress
callback to a separate function for better code visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by 73b8b7f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides my last comments!
src/components/ReportWelcomeText.tsx
Outdated
</> | ||
)} | ||
{isChatRoom && ( | ||
<> | ||
<Text>{roomWelcomeMessage.phrase1}</Text> | ||
{roomWelcomeMessage.showReportName && ( | ||
{roomWelcomeMessage.showReportName && !!report?.reportID && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikosamofa I think is better to do this condition inside onPress
callback instead of here, e.g.:
onPress={() => {
if (!report?.reportID) {
return;
}
Navigation.navigate(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report.reportID));
}}
You can also extract this onPress
callback to a separate function for better code visibility.
@nikosamofa Could you adjust the title of this PR so it aligns with the issue? |
Reviewer Checklist
Screenshots/VideosiOS: mWeb Safarin/a |
@nikosamofa You've added only screenshot/video for web to your PR's Author Checklist. Please add missing screenshots for other platforms. |
…migration/ReportWelcomeText
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikosamofa Thanks, I've also had an issue with iOS mWeb, so putting things together I think it's safe to say it's tested.
LGTM
@burczu Thanks for reviewing the PR |
…migration/ReportWelcomeText
yeah I can review and merge! @nikosamofa can you double check you're using the latest checklist from https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md in the PR description? The check isn't liking it. Thanks, will review shortly. |
Thanks @dangrous , I updated the author checklist from the link you provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
Bloop sorry you as well, @burczu , looks like we must have just updated the checklists - sorry! https://raw.githubusercontent.com/Expensify/App/main/contributingGuides/REVIEWER_CHECKLIST.md |
@dangrous Updated, but I wonder what happened? I usually copy the list from this comment: #33251 (comment) - if requirements has changed, shouldn't be this comment changed too? |
@blazejkustra There was a merge conflict, I resolved it and confirmed the component works fine. The checklist should be green after current checking. Thanks |
@dangrous Welcome back, could you review this PR and merge it if it's fine? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/dangrous in version: 1.4.21-0 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.21-4 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.21-4 🚀
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.21-4 🚀
|
Details
Fixed Issues
$ #25101
PROPOSAL: #25101 (comment)
Tests
ReportWelcomeText.js
.Offline tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop