Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.98k stars 2.98k forks source link

[$250] Distance - The route is cut off in the receipt in the split #55079

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.83-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5436429 Email or phone of affected tester (no customers): sustinov@applausemail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Open NewDot app
  2. Log in to the HT account
  3. Create any conversation or go to an existing one
  4. Create distance split with any valid route
  5. Go to the details of the created split

Expected Result:

The itinerary must fit completely on the receipt in the split section

Actual Result:

The route is cut off in the receipt in the split

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/81dab9cd-455d-4f5f-888c-0495e321a54e

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021877833668969168684
  • Upwork Job ID: 1877833668969168684
  • Last Price Increase: 2025-01-10
  • Automatic offers:
    • Krishna2323 | Contributor | 105728671
Issue OwnerCurrent Issue Owner: @eh2077
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Krishna2323 commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2025-01-10 17:25:34 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Distance - The route is cut off in the receipt in the split

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

  1. Add style={{height: '100%'}} to the receipt container. https://github.com/Expensify/App/blob/8abec8ee57c11cde994431d4d3630eeae0c995c5/src/components/MoneyRequestConfirmationListFooter.tsx#L708-L712
  2. Wrap MoneyRequestConfirmationList with ImageBehaviorContextProvider. https://github.com/Expensify/App/blob/8abec8ee57c11cde994431d4d3630eeae0c995c5/src/pages/iou/SplitBillDetailsPage.tsx#L107-L138
  3. Set the value for shouldSetAspectRatioInStyle prop to shouldSetAspectRatioInStyle={!TransactionUtils.isDistanceRequest(transaction)}.
                    <ImageBehaviorContextProvider shouldSetAspectRatioInStyle={!TransactionUtils.isDistanceRequest(transaction)}>
                        {!!participants.length && (
                            <MoneyRequestConfirmationList
                                payeePersonalDetails={payeePersonalDetails}
                                selectedParticipants={participantsExcludingPayee}
  4. Do the same in other similar components if the bug happens there.
  5. If we want then we can wrap the receipt directly in MoneyRequestConfirmationListFooter.tsx.

    What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?


    What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~021877833668969168684

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

mohit6789 commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The route is cut off in the receipt in the split.

What is the root cause of that problem?

Image is misaligned because height: auto applied to image. This issue is present in all kind of images not limited to route image https://url.upwork.com/_01MWuptBIbMYYLxz1t5qC6HaKUMyoQ5zHM.

This issue is occured because of this line.

What changes do you think we should make in order to solve the problem?

To fix problem we need to make following changes.

  1. Set the style={{height: '100%'}} to the pressable button here

  2. Make below changes here so aspect ratio does not set for all the images inside MoneyRequestConfirmationListFooter.

<ImageBehaviorContextProvider shouldSetAspectRatioInStyle={false}>
    <ReceiptImage
        isThumbnail={isThumbnail}
        // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
        source={resolvedThumbnail || resolvedReceiptImage || ''}
        // AuthToken is required when retrieving the image from the server
        // but we don't need it to load the blob:// or file:// image when starting an expense/split
        // So if we have a thumbnail, it means we're retrieving the image from the server
        isAuthTokenRequired={!!receiptThumbnail && !isLocalFile}
        fileExtension={fileExtension}
        shouldUseThumbnailImage
        shouldUseInitialObjectPosition={isDistanceRequest}
    />
</ImageBehaviorContextProvider>

This will fix issue for all type of image, not only for route image. By making this changes if we add new image type that will also be taken care autmatically.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

eh2077 commented 1 week ago

Reviewing proposals

eh2077 commented 1 week ago

@Krishna2323 's proposal looks good to me!

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @Julesssss, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

mohit6789 commented 1 week ago

@eh2077 don't you want to handle the issues in other image type?

Julesssss commented 1 week ago

Yeah ideally we resolve this elsewhere. This does seem to be briefly called out here:

Do the same in other similar components if the bug happens there.

eh2077 commented 1 week ago

Yeah ideally we resolve this elsewhere. This does seem to be briefly called out here:

Do the same in other similar components if the bug happens there.

Yes, I think @Krishna2323 's proposal already covered that idea.

  1. Do the same in other similar components if the bug happens there.
  2. If we want then we can wrap the receipt directly in MoneyRequestConfirmationListFooter.tsx.
melvin-bot[bot] commented 1 week ago

πŸ“£ @Krishna2323 πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

Krishna2323 commented 5 days ago

PR will be ready today.

melvin-bot[bot] commented 5 days ago

@Julesssss, @lschurr, @Krishna2323, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

eh2077 commented 5 days ago

Not overdue, we're waiting for the PR

Krishna2323 commented 4 days ago

@eh2077, PR is ready for review ^. The android app crashes as soon as it is launched, please let me know if you know something about it.

melvin-bot[bot] commented 2 days ago

Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.

Julesssss commented 2 days ago

Hey @dubielzyk-expensify,

Could you please take a look at this thread in the PR? We're trying to figure out if there is a preference for receipt alignment. We seem to use both center and top alignment.

This issue is specifically about the distance request alignment, but it touches on regular receipt alignment too. Thanks!

dubielzyk-expensify commented 1 day ago

Hmm. I thought we generally use top alignment on receipts but doing both seems weird. I'll let @Expensify/design chime in too, but I think we should either do center or top, not flip flop like the thread shows.

As for the distance request though, it feels like this shouldn't be an issue here if the map dimensions we pull in are sized to the thumbnail size. With receipts we have images that are portrait mode, but for a map, surely we can specify the dimensions, so I don't think we actually should run into any issues regardless of alignment. Happy to be told I'm wrong here though πŸ˜„

shawnborton commented 1 day ago

Agree that we like to top-align all image receipts.

For distance, I think it used to work perfectly fine before we made changes to align everything to the top. So maybe we just need to make distance receipts work exactly like they did a week ago, but make sure any image receipts from uploaded/scanned images are top-aligned. Could we do something like that?

dannymcclain commented 1 day ago

Agree with the above!

Julesssss commented 1 day ago

Sounds good, thanks all.