feat: [CFI] Add group-by report layout feature#72943
Conversation
|
@shubham1206agra 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] |
f61de24 to
adab45b
Compare
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Working on the unit tests. I will add them today/tomorrow. Until then @shubham1206agra can review how the is code looking with the feature request. |
|
@TaduJR did you forget the video recording as well? |
Oh no I didn't. I am working on the tests steps since I will recording on based on the test steps I wanted a confirmation from you to see if it is sufficient after I finished writing it |
This comment has been minimized.
This comment has been minimized.
|
🚧 @trjExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
Okay, can you please write the test steps and record he vids for the OP today? We're keen to get this shipped and out the door as customers are asking for it. @Expensify/design I'm kicking off a build in case you want to review in the meantime to speed up any design feedback. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
@trjExpensify This is problem with BE. BE needs to send the Onyx key in OpenApp call. |
|
Ah gotcha, so:
I'm cool to not block this PR on that, but we'll want to get that rectified pronto. Discussing that internally here with @nkuoch and @mountiny. |
|
The build is looking as expected to me - though I will admit I don't love it. I think it certainly accomplishes the goal for now, but I'm not sure how I feel about this being the default report layout forever haha. Not a blocker, just something I'm going to keep in mind to explore down the line. I also agree with Jason's point about finding a solution for totals, but ultimately feel the same as him here:
|
|
Wrote the test steps. Let me know what you think, and see if I missed case accidentally. |
|
@dannymcclain yeah maybe let's prioritize some explorations back in the lab? In the meantime, let's merge what we have and give our users what they want. |
Yeah, really good point. It feels pretty redundant on mobile when you can't see the totals. I feel like we need to show it all or not bother. FWIW, OldApp doesn't have this report layout stuff. |
Thank you for your feedback!
On it.
Will do that ASAP.
Awesome, I like this a lot.
Will do that as well. What have decided on this comment? |
|
@nkuoch Do you want to review this or do you want me to take this issue to the finish line? |
My pleasure 🙏. Thank you for your review. |
|
🚀 Deployed to staging by https://github.com/nkuoch in version: 9.2.64-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.64-5 🚀
|
|
🚀 Deployed to staging by https://github.com/nkuoch in version: 9.2.65-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.65-6 🚀
|










Explanation of Change
Explanation of Change
This PR implements a group-by report layout feature that allows users to organize expense report transactions by Category or Tag, matching the familiar OldDot functionality that NewDot customers have been requesting.
Key Changes:
Fixed Issues
$ #72100
PROPOSAL: #72100 (comment)
Tests
Desktop Testing
Category Grouping:
Tag Grouping:
Preference Persistence:
Offline Mode:
Edge Cases:
Mobile Testing (iOS/Android)
Category Grouping:
Group Selection:
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop