-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: use correct report field title for money request reports #34877
Conversation
@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@@ -53,8 +55,8 @@ function MoneyReportView({report, policyReportFields, shouldShowHorizontalRule}: | |||
StyleUtils.getColorStyle(theme.textSupporting), | |||
]; | |||
|
|||
const sortedPolicyReportFields = useMemo( | |||
() => policyReportFields.sort(({orderWeight: firstOrderWeight}, {orderWeight: secondOrderWeight}) => firstOrderWeight - secondOrderWeight), | |||
const sortedPolicyReportFields = useMemo<PolicyReportField[]>( |
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.
Is the <PolicyReportField[]>
syntax necessary?
const sortedPolicyReportFields = useMemo( | ||
() => policyReportFields.sort(({orderWeight: firstOrderWeight}, {orderWeight: secondOrderWeight}) => firstOrderWeight - secondOrderWeight), | ||
const sortedPolicyReportFields = useMemo<PolicyReportField[]>( | ||
(): PolicyReportField[] => policyReportFields.sort(({orderWeight: firstOrderWeight}, {orderWeight: secondOrderWeight}) => firstOrderWeight - secondOrderWeight), |
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.
policyReportFields.sort
Please use toSorted
!
I know that you just touched this part, but you're aware of what this code does, right? We're mutating the array inside the useMemo
builder.
src/languages/es.ts
Outdated
@@ -1907,6 +1907,7 @@ export default { | |||
report: { | |||
genericCreateReportFailureMessage: 'Error inesperado al crear el chat. Por favor, inténtalo más tarde', | |||
genericAddCommentFailureMessage: 'Error inesperado al añadir el comentario. Por favor, inténtalo más tarde', | |||
genericUpdateReportFieldFailureMessage: 'Error inesperado al actualizar el campo. Por favor, inténtalo más tarde', |
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.
Is this translation approved?
@@ -457,6 +458,19 @@ Onyx.connect({ | |||
callback: (value) => (allPolicies = value), | |||
}); | |||
|
|||
let allPolicyReportFields: OnyxCollection<PolicyReportFields>; |
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.
I hope it's not one of these collections we're nuking and un-nuking all over again...
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.
What do you mean @cubuspl42? This is a new collection that we built specifically for the report fields feature.
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.
There is (or might be?) a pattern that sometimes we add an Onyx-synchronized in-memory collection just to nuke it later for performance reasons. But I don't have any sources handy right now. Likely we'll be fine 🤷♂️
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.
Hm... The only reason why I used it like this was because this function is being used at a lot of places and getting this in all of those places might cause un-needed re-renders. Besides, I see that we're still doing this for report
and policies
.
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.
Yeah, it was a general comment, not a change request. This one doesn't need a resolution.
1c5466c
to
51c3baf
Compare
Bump on the review @cubuspl42 |
src/libs/ReportUtils.ts
Outdated
const reportFields = Object.entries(allPolicyReportFields ?? {}).find(([key]) => key.replace(ONYXKEYS.COLLECTION.POLICY_REPORT_FIELDS, '') === report?.policyID)?.[1]; | ||
const titleReportField = Object.values(reportFields ?? {})?.find((field) => field.type === 'formula'); |
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.
Could we extract one or two helper functions here? Long chain of invocations, giving it some name could help the readability
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.
Done.
correct-field-title-issue-1-web.mp4Am I doing something wrong? |
No. What's the issue here? |
That's expected because you have the beta turned on. We'll be optimising it further in #35043 |
Can we hold on that issue, or on something else? The current behavior doesn't seem to be working at all. I understand that it's meant to be iterative, but can't we settle on some stable iteration that doesn't result in glitches? |
I think holding this would be a little difficult because these changes are sort of required for that issue as well. Thinking about it again, I found it much easier to combine this change into my backend integration PR as it makes the whole flow much easier to test. As a result, I'm closing this PR in favour of #34483 |
Details
This PR edits the money request reports such that they use the correct report title if one is enforced by setting up the report fields.
Fixed Issues
$ #33131
PROPOSAL: N/A
Tests
policyID
with thepolicyID
of thepolicy
the opened report is attached to:canUseReportFields
beta inPermissions.ts
file.expense #reportID
.Offline tests
N/A
QA Steps
policyID
with thepolicyID
of thepolicy
the opened report is attached to:canUseReportFields
beta inPermissions.ts
file.expense #reportID
.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
Skipping because I'm having issues with android build.
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop