Multi files attachments carousel#65316
Conversation
Yes, will be on it in few hours. |
|
Noticed the close transition is jolty on reports page for PDF files
Screen.Recording.2025-07-02.at.9.28.52.at.night.mov |
|
File validation doesn't seem to be working properly, i can upload corrupt file. Screen.Recording.2025-07-02.at.9.55.04.at.night.mov |
|
We are showing error copy that's supposed to be for multi-file attachments when uploading an invalid file. Screen.Recording.2025-07-02.at.9.58.04.at.night.mov |
For this one we already have a bug reported. I will be working on this soon #65319 |
|
@getusha so far only these bugs? |
|
Yes seems to work well on web. will finalize my review tomorrow. |
|
I guess the other two bugs are rather from the previous PR as I didn't change the logic of validation, validation texts or transitions, only moved it, so IMHO shouldn't be the blockers here |
|
Any visual changes here as to need a @Expensify/design team review? |
|
yep, I'm working on it - I will fix the issues and push the changes from the reverted PR to this one |
@dubielzyk-expensify only if the carousel looks ok - but I will ping you when I resolve conflicts and fix other issues |
|
@getusha I've resolved conflicts and added missing code from the previous PR. None of the bugs fixed yet, but can you retest current changes (if it works the same as before the revert)? |
TBH I cannot recreate, but I'll try a few more times |
That sounds reasonable 👍 |
I do agree @getusha this is an important matter. I'll raise a discussion tomorrow in the #quality channel and will tag you there. |
I guess it's known issue for iOS that's why the loader was added here #59519 for request step scan. As we do a ton of calculations - validation, resizing, converting heic to jpeg it makes sense to use this loader, even if any performance improvements can be done, as we still can accept up to 30 files cc @lakchote |
|
so the issue with the delay was already been discussed #59347 |
|
One last thing, can we show the loading indicator only after we select a file? Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-07-08.at.18.54.47.mp4 |
We really should discuss again if there are better options, it's n times more noticeable. |
|
@getusha no - for 2 reasons: 1. I tried it but it doesn't work, 2. it should look consistent with the IOURequestStepScan flow |
|
Yes, discussion can only be beneficial here. We might want to keep the loader and also make performance improvements, too. I kinda like to make the user aware that an action is processing |
|
Yeah maybe show a loader on the menu item, if that's possible? current one looks tacky. |
It can always be done as a follow-up, I'm proceeding with the merge, and we'll adjust if needed |
but it won't be consistent with the Request step scan. And how I am supposed to stop this loader? This context was deliberately introduced to be able to start/stop loader form any place of the app. I need to have access to this in the hook, in the attachment picker and in the Report action compose |
cc @trjExpensify what do you think about this? In my opinion, we should be consistent with our flows. |
|
✋ 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/lakchote in version: 9.1.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.1.78-4 🚀
|
Explanation of Change
Because the AttachmentModal wasn't fully decoupled from the report attachment logic, but only AttachmentCarousel was I've decided to create a separate component for displaying files that are prepared to be sent in the composer.
I've moved all the logic that handles files validation to the new component and removed some code that because of creating a new component won't be used in AttachmentModal.
I also recommend creating a separate component for displaying receipts to handle only receipts' preview to simplify further AttachmentModal and make it only responsible for displaying report's attachments or previewing avatars. (Out of scope of this PR)
I haven't removed all the logic regarding files from AttachmentModal - I want to make testing simplier and I want to avoid having regressions. This can be done in a follow-up PR when the AttachmentComposerModal will be merged and tested thoroughly.
Fixed Issues
$ #59443
PROPOSAL: -
Tests
PREREQUISITES: all betas / newDotMultiFilesDragAndDrop beta enabled
[WEB / DESKTOP]
[MOBILE]
[CHECK THE ERROR HANDLING]
[CHECK THE SINGLE FILE FLOW]
Offline tests
n/a
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 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
Screen.Recording.2025-07-02.at.10.16.01.mp4
Android: mWeb Chrome
Screen.Recording.2025-07-02.at.10.13.54.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.-.2025-07-02.at.08.02.01.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.-.2025-07-02.at.10.25.13.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-07-02.at.09.24.46.mp4
Screen.Recording.2025-07-02.at.09.23.48.mp4
Screen.Recording.2025-07-02.at.09.23.28.mp4
MacOS: Desktop
Screen.Recording.2025-07-02.at.10.32.26.mp4