-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Use new transaction component inside reports in search #62286
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
Use new transaction component inside reports in search #62286
Conversation
@trjExpensify |
@DylanDylann 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] |
That makes sense to me. Can we double check our spacing here? It looks like these new rows have too much space between them when you view a report from Reports > Expense Reports. Are we close to a test build? I can grab more specific feedback once we fire one up. |
I prepared this component in a way that allows us to pass a list of columns we want to include depending on the context, and I’ve set it up so that the columns are the same as before. If we want to have the same columns as in the report, that’s also possible. In that case, do we also want to change the columns for the regular "Expense" tab? |
I don't think we want to change anything about how the columns were previously working on the Reports page, do we? If anything, we want this new component to mirror what we previously had on the Reports page. There are some optimizations we want to make with columns in the future (custom columns, conditionally showing/hiding them if they are populated or not, etc) but I think we can focus on that after this. |
The spacing has been fixed, and from what I understand, I kept the same column layout in the search as it was before. If you have any feedback, I’m happy to make adjustments. I think we can trigger an ad-hoc build so @shawnborton can take a look. |
Sounds good, will trigger the build now 🚀 |
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
transactionItem={transaction} | ||
isSelected={!!transaction.isSelected} | ||
dateColumnSize={dateColumnSize} | ||
shouldShowTooltip |
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.
Previously TransactionListItemRow
showTooltip
prop was based on showTooltip
. Why do we set it to always true here?
shouldShowTooltip | |
shouldShowTooltip={showTooltip} |
@@ -260,6 +275,16 @@ function MoneyRequestReportTransactionList({report, transactions, reportActions, | |||
shouldUseNarrowLayout={shouldUseNarrowLayout || isMediumScreenWidth} | |||
shouldShowCheckbox={!!selectionMode?.isEnabled || isMediumScreenWidth} | |||
onCheckboxPress={toggleTransaction} | |||
columns={[ | |||
CONST.REPORT.TRANSACTION_LIST.COLUMNS.RECEIPT, |
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.
Can we define this array as a const
and put it at the top of the file?
shouldShowCheckbox={!!canSelectMultiple} | ||
onCheckboxPress={() => onCheckboxPress?.(transaction as unknown as TItem)} | ||
columns={[ | ||
CONST.REPORT.TRANSACTION_LIST.COLUMNS.RECEIPT, |
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.
Same here ☝️
This comment has been minimized.
This comment has been minimized.
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Nice, that seems to have fixed it for me. Let's get this into final review then? |
@DylanDylann Hi, could you take a look at this PR? It seems that all the related bugs have been resolved |
@@ -59,6 +60,23 @@ type TransactionWithOptionalHighlight = OnyxTypes.Transaction & { | |||
shouldBeHighlighted?: boolean; | |||
}; | |||
|
|||
type TransactionWithOptionalSearchFields = TransactionWithOptionalHighlight & { |
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.
This type isn't used in this file. Please move this type of definition to src/components/TransactionItemRow/index.tsx
@sumo-slonik Could you also resolve conflict? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-05-21.at.12.49.00.movAndroid: mWeb ChromeScreen.Recording.2025-05-21.at.12.45.18.moviOS: HybridAppScreen.Recording.2025-05-21.at.14.15.04.moviOS: mWeb SafariScreen.Recording.2025-05-21.at.12.44.28.movMacOS: Chrome / SafariScreen.Recording.2025-05-21.at.12.43.06.movMacOS: DesktopScreen.Recording.2025-05-21.at.12.48.02.mov |
…ent_in_search # Conflicts: # src/components/SelectionList/Search/ReportListItem.tsx
@DylanDylann done 💥 |
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.
One nab comment
I think we also need to use this new row style for the Reports > Expenses page on mobile too. We can do that as a follow up. But once we do that, then we really only have one row style to maintain. |
I agree and I’m happy to take care of it. However, I’ll be away from tomorrow until Tuesday, so I can either handle it when I’m back or ask someone else to follow up in the meantime." |
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!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Created a follow up issue here: #62474 |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.48-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.48-0 🚀
|
const columnComponent: ColumnComponents = useMemo( | ||
() => ({ |
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.
NAB: did we pull these column rendering here inside a useMemo
for performance reasons?
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.
yup, it should stop react from remounting components
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.49-5 🚀
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.1.50-0 🚀
|
Explanation of Change
I added the transaction component — the same one used in the report — to the "group by report" view in the search, and refactored it to make it more flexible for potential reuse elsewhere.
Fixed Issues
$ #61512
PROPOSAL:
Tests
Go to the Search tab, then to Group by Reports, and check if the transactions are displayed correctly.
Offline tests
Unnecessary
QA Steps
Same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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
Screen.Recording.2025-05-19.at.17.05.20.mov
iOS: Native
Screen.Recording.2025-05-19.at.16.55.36.mov
iOS: mWeb Safari
Screen.Recording.2025-05-19.at.17.00.20.mov
MacOS: Chrome / Safari
Screen.Recording.2025-05-19.at.16.26.17.mov
MacOS: Desktop
Screen.Recording.2025-05-19.at.16.36.47.mov