-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD] [$250] [MEDIUM] Smartscan: Receipt thumbnail should show the top of the receipt #34120
Comments
Job added to Upwork: https://www.upwork.com/jobs/~019bb6b442f89fba02 |
Triggered auto assignment to @adelekennedy ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Showing the middle portion of the receipt What is the root cause of that problem?For What changes do you think we should make in order to solve the problem?Make the View wrapping the image to have overflow hidden, for the image itself, it will take full width, variable height according to the The View wrapper already has overflow hidden here so all we have to do is to make sure our
For native platforms, we should do similarly in the We need to double check all possible cases of showing thumbnail receipt and add What alternative solutions did you explore? (Optional)Another approach is to I explored using |
ProposalPlease re-state the problem that we are trying to solve in this issue.Receipt thumbnail is not showing the top of the image What is the root cause of that problem?For displaying a receipt What changes do you think we should make in order to solve the problem?For always displaying the top section of receipt thumbnail, the image container width needs to be set to the width of thumbnail and the height should be set as per aspect ratio of the image. This makes sure that the thumbnail image gets rendered fully and not get scaled or repositioned due to 'cover'. function ThumbnailImage({previewSourceURL, style, isAuthTokenRequired, imageWidth = 200, imageHeight = 200, shouldDynamicallyResize = true}: ThumbnailImageProps) {
// Use a variable to save container width on layout
const [containerSize, setContainerSize] = useState({ width: 0, height: 0 });
...
// Now compute the aspect ratio of the image
const aspectRatio = currentImageWidth && currentImageHeight ? currentImageWidth / currentImageHeight : 1;
// Compute aspect ratio of container
const containerAspectRatio = containerSize.width / containerSize.height;
// Get static image size as per aspect ratios
const getImageSize = useCallback(() => {
if (!aspectRatio) {
return [styles.w100, styles.h100]
}
if (aspectRatio > containerAspectRatio) {
return [{
height: containerSize.height,
width: containerSize.height * aspectRatio
}]
}
return [{
width: containerSize.width,
height: containerSize.width / aspectRatio
}]
}, [aspectRatio, containerAspectRatio, containerSize.height, containerSize.width, styles.h100, styles.w100])
const sizeStyles = shouldDynamicallyResize ? [StyleUtils.getWidthAndHeightStyle(currentImageWidth ?? 0, currentImageHeight)] : getImageSize();
return (
<View
style={[style, styles.overflowHidden]}
// Get container width
onLayout={(e) => setContainerSize(e.nativeEvent.layout)}
>
<View style={[...sizeStyles, styles.alignItemsCenter, styles.justifyContentCenter]}>
<ImageWithSizeCalculation
url={previewSourceURL}
onMeasure={updateImageSize}
isAuthTokenRequired={isAuthTokenRequired}
/>
</View>
</View>
); Above logic also handles images whose width is larger than height. Also the logic works for both native and web platforms. There are a couple of screens like MoneyRequestConfirmationList and ReportActionItemImage etc where Image component is used instead of ThumbnailImage for displaying receipt. Such instances need to be replaced so that all occurrences of receipt image is properly displayed Result iOS fix-ios_out.mp4Web fix-web_out.mp4What alternative solutions did you explore? (Optional)None (edited) |
@aswin-s FYI your solution's the same as one of my alternate solution
I'm writing the implementation details for my proposal and updating soon |
ProposalPlease re-state the problem that we are trying to solve in this issue.Receipt thumbnail is not showing the top of the image What is the root cause of that problem?Inside What changes do you think we should make in order to solve the problem?We need to use Test branch - https://github.com/shubham1206agra/App/tree/test-thumbnail Note - Naming of function is not final. What alternative solutions did you explore? (Optional)NA |
Noted @dukenv0307. My proposal has just one approach detailed which works on all platforms. I've tested on both iOS and web and shared the screen grabs as well. I've also addressed scenarios like serving local image when receipt upload is in progress in the above video. So it is not exactly same or abstract like your proposal. I'll leave the merit of the proposal to the C+. |
Don't worry. C+ will take care of these things :) |
I'll first ask about your main solution.
I assume you mean the Do I understand correctly that you suggest modifying the logic of Are Do I understand correctly that you, like @aswin-s, suggest adjusting the math of |
I have a prop to control that
No, its image orientation.
Yes |
Proposal updated to include implementation details.
@cubuspl42 I tested and it doesn't seem to work on all supported platforms so I updated my proposal with a custom prop so it can be reusable across all images component, just like FYI my solution is a generic one that will work for all occurences of |
@cubuspl42 Yes my proposal is to modify the logic in Thumbnail component is used currently for Image attachment previews and Receipt previews. For image attachments in ReportAction, we always show the entire image in the thumbnail and In my proposal image alignment is changed only when |
@shubham1206agra Thank you. Let's use Markdown code syntax for technical and code identifiers only, as it can create confusion otherwise. @aswin-s You accidentally double-sent your comment |
@aswin-s Your analysis of the @shubham1206agra The code branch is, in my opinion, a helpful tool for sharing proof of basic testing and specifics of the proposal. But, like on StackOverflow: links are considered helpful and desired additions, but link-only answers are against the guidelines. So far, your solution seems (especially without looking at the code diff) very similar to the one by @aswin-s. Unless you handle some critical and objective case(s) that the other solution doesn't, I don't think it is a substantial improvement. |
@cubuspl42 what do you think about my proposal? I don't think @aswin-s 's ThumbnailImage solution will work for receipts that don't use |
@dukenv0307 I'm confused as to why you started with a pure CSS solution and switched to a cross-platform one late. I haven't yet analyzed the updated solution.
This is an important observation. I missed this fact. Indeed, there's some conditional logic in When is the receipt a local image? How does alignment work now in such a case? |
@cubuspl42 I've mentioned about handling other receipt images in my proposal.
I also mentioned about it in my response to dukenv0307
When the receipt image is getting uploaded it shows image url from local device or from blob (web platform). It gets handled over here. App/src/components/ReportActionItem/ReportActionItemImage.js Lines 79 to 82 in a4bdebe
And for confirmation page over here. App/src/components/MoneyRequestConfirmationList.js Lines 607 to 616 in a4bdebe
Both these places use Image component to show the thumbnail at the moment. So if we want the same behaviour as in chat, these should be replaced with Also the iOS video recording shows this scenario handled on all screens. |
@cubuspl42 as mentioned here, my alternate solution in the initial proposal is the So I updated my proposal in here with the implementation details of my initial
@cubuspl42 So with this the logic is now coupled with the With my proposal the the So the issue has nothing to do with |
ProposalPlease re-state the problem that we are trying to solve in this issue.Receipt thumbnail should show the top of the receipt. What is the root cause of that problem?We are using What changes do you think we should make in order to solve the problem?Smallest change to make in order to fix this issue is to replace |
The idea of this solution was (barely) mentioned before in the proposal by @dukenv0307. What is being reviewed, though, is a proposal as a whole. The main proposed solution, at the time of my initial review, was fundamentally HTML-specific (it mentioned Also, @dukenv0307 disagrees with the mentioned idea, so I wouldn't like to force them to implement it:
(the claim wasn't backed by measurements or any other source, though) The proposal by @aswin-s contained solely the Thanks, everyone! As is often the case, other proposals also contained solutions that could work. Remember that the order in which the proposals are posted is not the main selection criterium. |
Triggered auto assignment to @Beamanator, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Upwork job price has been updated to $500 |
@Santhosh-Sellavel reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks |
@Santhosh-Sellavel We're in a follow-up PR here #40763 to also fix the bug for the confirmation page as well. |
@dukenv0307 it looks like we've been showing the top of receipts for distance request receipts as well, but we don't want that behavior - see #40791 Also most likely I'll have to adjust my backend fix to only send receipt thumbnails focused on the top center IFF the receipt is NOT a distance request 🤔 |
Upwork job price has been updated to $250 |
@adelekennedy dropping to $250 for a regression. |
@Beamanator, @adelekennedy, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@mallenexpensify/@ad Can you assign me here please, seems I got unassigned accidentally
|
Done, added @Santhosh-Sellavel , sorry about that |
@Beamanator, @adelekennedy, @Santhosh-Sellavel, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@Beamanator, @adelekennedy, @Santhosh-Sellavel, @dukenv0307 Still overdue 6 days?! Let's take care of this! |
Hmm @dukenv0307 your PR went to prod a week or so ago, are we waiting on anything here? |
@Beamanator We're waiting for payment here. |
Looks like this hit production on 4/23, so gonna pay Contributor: @dukenv0307 paid $250 via Upwork |
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: 1.4.22-6
Reproducible in staging?: yes
Reproducible in production?:.yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1704757554079199
Action Performed:
Expected Result:
Should be showing from the top of the receipt
Actual Result:
Showing the middle portion of the receipt
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: