Add selection to table on MoneyRequestReport page #59433
Conversation
…08-add-checkboxes-to-table
…08-add-checkboxes-to-table
…dd-checkboxes-to-table
|
@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] |
|
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
|
@jnowakow please add test steps |
This comment has been minimized.
This comment has been minimized.
| useEffect(() => { | ||
| clearErrors(ONYXKEYS.FORMS.MONEY_REQUEST_HOLD_FORM); | ||
| clearErrorFields(ONYXKEYS.FORMS.MONEY_REQUEST_HOLD_FORM); | ||
| }, []); |
There was a problem hiding this comment.
won't there be any other dependency to clear this error?
There was a problem hiding this comment.
We want to clear them only when leaving page that page. It's handled that way on HoldReasonPage as well.
|
I think the gap between checkbox and receipt icon is too large: This is what the reports page looks like for reference: Is there any way to harmonize styles between these components? My fear is that we need to maintain two separate row components that basically need to look exactly the same. |
|
The gaps between rows is now too large: From staging, I see this which is correct: |
|
The table header is too close to the top of the view as well: When is the last time we merged in main? Maybe these changes are available and we just need to merge them in? |
|
🚧 @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! 🧪🧪 |
|
Hmm looks like we're still off by 2px: The report itself has more padding than the reports page. Any idea what's going on? |
There was a problem hiding this comment.
Tests well for me and changes look good to me, but I would still wait for the design feedback from @shawnborton to be addressed unless he is happy to move on. This PR does hold some other changes so unless its some biggest design issues, I would recommend merging 🙌
|
Looking good outside of my comment above about the header being slightly off, and only other comment is that if something is selected, it should not have any :hover styles - it should just keep the selectedBG color on hover. CleanShot.2025-04-07.at.18.32.09.mp4 |
Co-authored-by: Vit Horacek <[email protected]>
|
This feature is actively worked on so I would said it's good to merge this PR and fix those two UI issues in one of coming PRs |
|
Fine by me! |
|
@shawnborton thank you! Added them here in case you could just add a bit more detail so its easier for folks to squash quickly 🚀 #59452 (comment) thank you! |
|
✋ 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/mountiny in version: 9.1.24-2 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
|
@shawnborton I've wanted to create follow up and add selection to mobile but I can't find designs. It should look like this: |
I think it makes sense to follow what we have for the reports page, where the whole header is replaced when we enter into select mode. cc @Expensify/design for the gut check there though! |
|
And yes, we need to add the checkboxes to the mobile expense rows. Again, use the Reports page as your guiding light! |
|
Agree with Shawn above! |
| const anyTransactionOnHold = selectedTransactions.some(isOnHoldTransactionUtils); | ||
| const allTransactionOnHold = selectedTransactions.every(isOnHoldTransactionUtils); | ||
|
|
||
| if (!anyTransactionOnHold && selectedTransactions.length === 1) { |
There was a problem hiding this comment.
Why was not the hold option shown when bulk selecting? Adding this line caused #60111
There was a problem hiding this comment.
I can't find conversation on slack because it was discussed in long thread but it was agreed that we don't want to iterate over selected issues and send many requests. The go to solution was to temporarily disable that option if many transactions are selected and add it when there's api to hold many transaction in one request.
There was a problem hiding this comment.
We have a BulkHoldRequest API in place, so we can remove this limitation and start working on a fix for that.
There was a problem hiding this comment.
@luacmartins great! Is there issue for it? I can replace it
There was a problem hiding this comment.
I don't think we have an issue yet, but I think @Tony-MK might be working on that
@Kicu @mountiny
Explanation of Change
Follow up to new MoneyRequestReport page. It adds selection to transactions table. The implementation lack support for long press selection on mobile devices, it will be added in next follow up in order to keep PRs small.
Fixed Issues
$ #57508
$ #59459
PROPOSAL: N/A
Tests
Selection doesn't work on small screen. It will be fixed in separate PR.
x selectedbutton with dropdown in headerOffline tests
x selectedbutton with dropdown in headerQA Steps
Selection doesn't work on small screen. It will be fixed in separate PR.
x selectedbutton with dropdown in headerPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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