Update create expense confirmation page to more clearly indicate automation#75196
Update create expense confirmation page to more clearly indicate automation#75196parasharrajat wants to merge 21 commits intoExpensify:mainfrom
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@allroundexperts 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] |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Nice, excited for this change!
|
I think we can set a max-height on the receipt, but we'll generally want it to fill on mobile screens. Then let's run a test build when you're ready |
|
So wouldn't it be better to keep the image height when the image is smaller. I will make that change then show you. |
|
@parasharrajat let me know when you want me to create the ad-hoc. |
|
@parasharrajat how's this going? What do we need to do to get it to the finish line? |
|
@parasharrajat What's the status here? |
|
I am trying to set the auto aspect ratio on the receipt image. I will let you know in sometime how did it go? |
|
I attempted to change the aspect ratio of images, but it is still not looking correct IMO. The original design is not correct as it stretches the image. This is how it will look for smaller images. But after testing a couple of images, I can see that most of them are wide images and in a 16:9 ratio. It means the height of these images will never expand vertically to take up the full space, even on mobile. It is because the width is small than the height of the mobile devices. So I want to get suggestions before I get more into it. @dubielzyk-expensify What do you think?
I will continue looking in the meantime. |
|
Yes, the phone will have different ratios, so while taking photos with the phone, it will be fine. Now my question remains the same. Are we fine with showing the image in original aspect ratio, or do we need to stretch it vertically up to max height even though the image will be clipped from sides? |
Curious for @dannymcclain ''s take here as well. Initially I was thinking stretching, but if it's landscape it will look very zoomed in. Maybe it's worth going with the original aspect ratio. What do you think Danny? |
|
@dubielzyk-expensify I'm torn—obviously we don't want to stretch the image so much that the quality looks really bad, but the effect doesn't really land as well when the image doesn't take up the whole area. Would it be insane to do something like this (which I've seen as a solution to this problem in other apps):
Basically what I'm doing is stretching the image in the BG and adding a blur/transparency and then setting the actual on top confined to its aspect ratio. Could be a terrible idea! |
|
I can experiment with this idea. |
|
Ohh I like that, Danny. Thanks for trying it out and helping us get to the best solution, @parasharrajat ! |
|
I do not have the dimensions of the receipt. @dannymcclain can you please share that? What is the max height you were referring to? |
|
I actually think if we go this route we won't need a max-height? Because we can always have the blurred image in the background to fill up whatever "extra" space there is. @dubielzyk-expensify what do you think? |
|
Agree. Let's try Danny's suggestion which means we'll fill the height regardless |
|
@parasharrajat needs to fix conflicts and merge main to fix checks |
|
I am currently not available for this so I have asked for help on slack and @ShridharGoel will take over. |
|
@puneetlath Do you think a temp branch can be created here in Expensify/App where we can merge this PR, and then I can open a PR which will have it's base as that temp branch? It will be easier to manage the git author history of the changes. |
|
Though ideally we should have merged #72002 in a temporary branch and then continued this, since it seems the commits are from there |
|
@ShridharGoel its fine. you can override. |
|
Being continued here: #78859 |





Explanation of Change
Fixed Issues
$ #71601
PROPOSAL:
Tests
Offline tests
Same as tests
QA Steps
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