[HOLD] [$250] [MEDIUM] Smartscan: Receipt thumbnail should show the top of the receipt

[m-natarajan]

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:

1. create an IOU using receipt scanning
2. Observe the thumbnail of the receipt

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?

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019bb6b442f89fba02
• Upwork Job ID: 1744585981661257728
• Last Price Increase: 2024-04-24
• Automatic offers:
• cubuspl42 | Reviewer | 28117185
• dukenv0307 | Contributor | 28117186

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot[bot]]

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

[dukenv0307]

Proposal

Please 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 resizeMode cover, the image will have default positioning which is in the middle. The behavior that we want is similar to the object-position: top in CSS, but this React Native doesn't support it out of the box so we need to add it to React Native.

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 aspectRatio (not fixed height).

The View wrapper already has overflow hidden here so all we have to do is to make sure our Image component can handle the object position top case.

1. Add a objectPositionTop boolean prop to our Image component to control this behavior (later we can modify to support other positions like bottom). I intended to make it as similar and reusable as possible (similar usage as native object-position: top in CSS), we just need to modify in common Image component and we can use it everywhere we use images in the app.
2. Add the prop to ImageWithSizeCalculation as well and pass it through to the inner Image, In here, add objectPositionTop because we want that behavior for ThumbnailImage. We can make it a prop of ThumbnailImage as well so we can control the position of ThumbnailImage.
3. Then handle in Image so the objectPositionTop property works Here

   const [aspectRatio, setAspectRatio] = useState();

Here

RNImage.getSize(source.uri, (width, height) => {
    onLoad({nativeEvent: {width, height}});

    if (props.objectPositionTop) {
        setAspectRatio(height ? width / height : 'auto');
    }
});

Here

style={[forwardedProps.style, aspectRatio !== undefined && {aspectRatio, height: 'auto'}]}

For native platforms, we should do similarly in the index.native file, for native platforms, we won't get the image dimensions unless we load the image, we might want to use Image.getSize similar to on web to make sure it's independent from the image loading, and then we can prevent displaying of the image while the aspectRatio is not yet available, it might help prevent some potential issues.

We need to double check all possible cases of showing thumbnail receipt and add objectPositionTop to those places as needed.

What alternative solutions did you explore? (Optional)

Another approach is to translate the image vertically an amount such that instead of appearing from the center, image will appear at the top, this will require some math to calculate the translation so I prefer the aspectRatio approach above

I explored using object-position: top and background-position: top as well but react native does not yet support it.

[aswin-s]

Proposal

Please 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 <ThumbnailImage/> component is used with shouldDynamicallyResize prop set to false. This causes the image to fill to the given container width and height. However the ImageWithSizeCalculation component covers the image to available width and height. This ensures that the displayed image properly covers the parent container. However a side effect of cover is that the image gets scaled and positioned such that it entirely covers the container, there by causing the vertical or horizontal alignment of image to be adjusted as needed.

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

https://github.com/Expensify/App/assets/6880914/2c14b40c-990b-4056-a262-2c7f39960b48

Web

https://github.com/Expensify/App/assets/6880914/615f7472-e5b7-46a2-b877-11bd17426f65

What alternative solutions did you explore? (Optional)

None (edited)

[dukenv0307]

@aswin-s FYI your solution's the same as one of my alternate solution

A potential alternative is to make the View wrapping the image to have overflow hidden, for the image itself, it will take full width, variable height according to the aspectRatio (not fixed height).

I'm writing the implementation details for my proposal and updating soon

[shubham1206agra]

Proposal

Please 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 ReportActionItemImage, the ThumbnailImage component is being used. But this component uses 100% for height and width, which makes image center (looks like center but not exactly).

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

We need to use aspectRatio technique to properly scale the images so that always top can be shown. We need to correctly scale the images for landscape, and portrait images.

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

[aswin-s]

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+.

[shubham1206agra]

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 :)

[cubuspl42]

@dukenv0307

I'll first ask about your main solution.

object-position: top

I assume you mean the object-position CSS property. How would this solve the problem on all supported platforms?

@aswin-s @shubham1206agra

Do I understand correctly that you suggest modifying the logic of ThumbnailImage, so it always aligns to the top? Does this component have other usages, where we might prefer different alignment?

@shubham1206agra

Are landscape and portrait some code / CSS constants?

Do I understand correctly that you, like @aswin-s, suggest adjusting the math of ThumbnailImage, but in a different way?

[shubham1206agra]

Do I understand correctly that you suggest modifying the logic of ThumbnailImage, so it always aligns to the top? Does this component have other usages, where we might prefer different alignment?

I have a prop to control that shouldShowTop, if it is false, then it will revert to older configuration (100% one).

Are landscape and portrait some code?

No, its image orientation.

Do I understand correctly that you, like @aswin-s, suggest adjusting the math of ThumbnailImage, but in a different way?

Yes

[dukenv0307]

Proposal updated to include implementation details.

I assume you mean the object-position CSS property. How would this solve the problem on all supported platforms?

@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 object-position: top.

FYI my solution is a generic one that will work for all occurences of Image, it's not relying on the ThumbnailImage implementation

[aswin-s]

@aswin-s @shubham1206agra

Do I understand correctly that you suggest modifying the logic of ThumbnailImage, so it always aligns to the top? Does this component have other usages, where we might prefer different alignment?

@cubuspl42 Yes my proposal is to modify the logic in ThumbnailImage component so that the image aligns to top when shouldDynamicallyResize is set to false.

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 shouldDynamicallyResize is always set to true. For receipt thumbnails the prop shouldDynamicallyResize is always set to false so that the thumbnail size is always fixed irrespective of the receipt image size.

In my proposal image alignment is changed only when shouldDynamicallyResize is set to false. So image attachment thumbnails in chat will remain unaffected. There is not enough differences in implementation to warrant for a completely different component like ReceiptThumbnail.

[cubuspl42]

@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

[cubuspl42]

@aswin-s Your analysis of the shouldDynamicallyResize usage was helpful and important in the context of this issue, thanks. If shouldDynamicallyResize = false is used for and only for receipt previews, we can just modify this code path in the YAGNI spirit.

@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.

[dukenv0307]

@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 ThumbnailImage to display (like local images). And we shouldn't replace those with ThumbnailImage because ThumbnailImage has a lot of overhead, and we don't need such replacement to fix this issue.

[cubuspl42]

@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.

ThumbnailImage solution will work for receipts that don't use ThumbnailImage to display (like local images)

This is an important observation. I missed this fact. Indeed, there's some conditional logic in ReportActionItemImage.

When is the receipt a local image? How does alignment work now in such a case?

[aswin-s]

@cubuspl42 I've mentioned about handling other receipt images in my proposal.

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

I also mentioned about it in my response to dukenv0307

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.

When the receipt image is getting uploaded it shows image url from local device or from blob (web platform). It gets handled over here.

https://github.com/Expensify/App/blob/a4bdebe94750e438fe22a7f569e68b54d37df1d0/src/components/ReportActionItem/ReportActionItemImage.js#L79-L82

And for confirmation page over here.

https://github.com/Expensify/App/blob/a4bdebe94750e438fe22a7f569e68b54d37df1d0/src/components/MoneyRequestConfirmationList.js#L607-L616

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 ThumbnailImage component. That's what I mentioned in my proposal towards last.

Also the iOS video recording shows this scenario handled on all screens.

[dukenv0307]

@dukenv0307 I'm confused as to why you started with a pure CSS solution and switched to a cross-platform one late

@cubuspl42 as mentioned here, my alternate solution in the initial proposal is the aspectRatio approach (similar to what others proposed later above). At the time you reviewed I was still trying to get the object-position solution working because if possible it will be the cleanest, but I couldn't find any since RN doesn't support it.

So I updated my proposal in here with the implementation details of my initial aspectRatio solution.

aswin-s: 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 ThumbnailImage component. That's what I mentioned in my proposal towards last.

@cubuspl42 So with this the logic is now coupled with the ThumbnailImage component and anywhere we use receipt we now have to convert to ThumbnailImage, which has unnecessary overhead compared to the Image component in those cases.

With my proposal the the objectPositionTop logic is contained within the Image component and will work well for all those cases (no need for all-to-ThumbnailImage refactor).

So the issue has nothing to do with ThumbnailImage component (it's not the root cause), it's just how resizeMode: cover behaves for Image in react-native, thus we should fix it there at the root.

[DanutGavrus]

Proposal

Please 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 width: 100% and height: 100% in combination with resizeMode: cover which crops the image to the middle, both vertically and horizontally.

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 height: 100% with height: *actual height of the thumbnail* in ReportActionItemImage for the ThumbnailImage component. The overflow is hidden, and the image will automatically start from the top with no other changes. If we want to make this change in other places too, we can generalise this approach. In order to obtain the actual height of the image in ReportActionItemImage we may use: RNImage.getSize(`${thumbnail}?authToken=${NetworkStore.getAuthToken()}`, (width, height) => {}) where RNImage is imported directly from react-native like import {Image as RNImage} from 'react-native'.

[cubuspl42]

~C+ reviewed 🎀 👀 🎀~

~I approve the proposal by @aswin-s~

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 div tags and pure CSS properties), so this is a big minus for that one in my opinion.

Also, @dukenv0307 disagrees with the mentioned idea, so I wouldn't like to force them to implement it:

And we shouldn't replace those with ThumbnailImage because ThumbnailImage has a lot of overhead

(the claim wasn't backed by measurements or any other source, though)

The proposal by @aswin-s contained solely the ThumbnailImage aspect ratio solution, going into a reasonable level of detail.

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.

[melvin-bot[bot]]

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

[dragnoir]

@cubuspl42 I checked the solution created by aswin and it's causing issues on the UI, pls check video below:

https://github.com/Expensify/App/assets/12425932/7dd6a68e-6678-4ee2-a9d8-56e927baaae2

Also don't you think this is a playaround? We are facking the details for the Child Image component. As example, currently what ever you pass to resizeMode, it will always be the same result.

Please bear with me as I pay close attention to the details :)

[shubham1206agra]

In that case, @aswin-s take my solution given in branch. :)

[cubuspl42]

@dragnoir

I wonder why this effect is not visible on the videos @aswin-s posted. Could it be related to caching? I agree that it doesn't look good. Also, on your video, the receipt image re-layouts three times (I could understand two). We'll have to

@shubham1206agra

I assume they have their own, just not shared

[shubham1206agra]

@shubham1206agra

I assume they have their own, just not shared

Looks like @dragnoir have the solution posted https://github.com/dragnoir/App/commit/1352857bef7dad999196e5f7b788e4c7aa6bf9ea.

[dragnoir]

@cubuspl42 if you console.log the sizeStyles (new style applied to the View container) console.log('sizeStyles: ', sizeStyles); You will see the below

As you can see, the new height will be different from the time the component render to the final state. I opened on the screenshot 3 values of the sizeStyles.

For theose values, from time component mount to final state: 1- We give to the Image (Child) component a height equal to 0 Result: resizeMode is on cover and will show the center.

2- Height new value but not the correct one, Result: change position

3- Final value of height

My prosal is similar to eswin but not loading issues. I calculate the new values with onLoadEnd and I do not ignore the resizeMode prop.

[dukenv0307]

@cubuspl42 would you mind giving some feedback on my proposal? Just curious why you think it's not a better solution.

(the claim wasn't backed by measurements or any other source, though

@cubuspl42 The ThumbnailImage has extra code compared to the Image, now we're converting the Image to ThumbnailImage, don't you think it's unnecessary overhead? (While we can keep using that Image with my solution)

Also my solution doesn't have the UI glitch that @dragnoir mentioned.

cc @Beamanator

[cubuspl42]

@dukenv0307

Just curious why you think it's not a better solution.

Isn't that a question that any author of any of the other proposals could've asked? There will always be some arguments for a given solution, and selecting a specific one is often a matter of taste and personal engineering experience.

ThumbnailImage has extra code

That code does something, not much different from what you're supposed to add to Image. Taking into consideration the component depth in our app (and in any modern web app, for what it's worth), one layer more or less is not that big of a difference.

ThumbnailImage, as opposed to Image, always has known width and height (as it has defaults used in case these values aren't passed explicitly), so I expected it to be a better candidate for implementing the discussed logic. Possibly, contrary to the findings from @dragnoir; I'm not sure yet, as two videos have different behavior.

@aswin-s FYI your solution's the same as one of my alternate solution

my solution doesn't have the UI glitch

So is the ThumbnailImage / aspectRatio your solution or isn't it? So far, it feels like you're juggling it and changing stances, whatever is more suitable in a given second. Arguing hardly against a solution present in one's own "What changes do you think we should make in order to solve the problem?" section is not serious, in my opinion.

Finding a technical issue in another contributor's solution and summaring the findings, so we can all settle on the best solution: GOOD ✅

Undermining the C+ decision because "my solution is mine and it's better", pinging the internal engineer for that reason: NOT GOOD ❌ (might result in a warning in the future)

[aswin-s]

@cubuspl42

I wonder why this effect is not visible on the videos @aswin-s posted. Could it be related to caching? I agree that it doesn't look good. Also, on your video, the receipt image re-layouts three times (I could understand two). We'll have to

I checked above issue in my implementation and yes it definitely happens during first render. I didn't notice this in my proposal because it happens only if you turnoff caching in network panel. This reason is that image needs to be rendered once for the native dimensions to be calculated. On web, by the time the layout measurements are available first render is already finished. Then the aspectRatio gets calculated and image is re-rendered with optimal size. So technically this cannot be avoided. However the image could be hidden from view and the loader could be shown until the aspectRatio calculation is complete by setting opacity or showing the loader. This is just an implementation detail that could be handled in the PR.

[DanutGavrus]

@aswin-s @cubuspl42

This reason is that image needs to be rendered once for the native dimensions to be calculated.

Just a note: maybe you could make use of getSize from react-native's Image, which returns the dimensions so we know them in the first render too. If we want to avoid calling getSize more than once we may also use useMemo.

Also, from what I've noticed, if we end up knowing the dimensions from the start, we may keep our Image component as is, and just replace height: 100% with the actual value.

[cubuspl42]

@Beamanator Let's wait with the assignment; we have to figure things out here first

[cubuspl42]

@aswin-s

This is just an implementation detail that could be handled in the PR.

Maybe, maybe not. It complicates the solution to the point where other solutions might be more reasonable. I'll have to re-evaluate.

[aswin-s]

@aswin-s

This is just an implementation detail that could be handled in the PR.

Maybe, maybe not. It complicates the solution to the point where other solutions might be more reasonable. I'll have to re-evaluate.

@cubuspl42 Sure no problem. Just want you to consider that all receipt images are of varying sizes and without knowing their native resolution it cannot be resized to parent container keeping the aspect ratio. So any viable proposal will need to get the native resolution first and then set the container / image size accordingly. And in the process the image needs to be rendered once.

@Beamanator The getSize method exported by ReactNative uses Fresco image library under the hood. It has some known issues loadinghigh resolution images. For native platforms we are using expo-image which uses Glide image library. So not sure if both will return the same image dimensions for high resolution images.

[cubuspl42]

So any viable proposal will need to get the native resolution first and then set the container / image size accordingly.

@dukenv0307 suggests otherwise...

Also my solution doesn't have the UI glitch that @dragnoir mentioned.

Guys, I'm getting tired here. I want to assign fairly, but I'm also assigned to multiple issues and can't get stuck here. I know you tested your solutions. Please just share the branches, and I'll give it a look locally.

[aswin-s]

@cubuspl42 Here is my branch. https://github.com/aswin-s/App/tree/fix/issue-34120

Also full disclosure I've updated my proposal to adjust the aspect ratio calculation so that it can accomodate multiple receipts stacked together. The core idea of the proposal is still the same so that it doesn't conflict with any other proposals.

https://github.com/Expensify/App/assets/6880914/7130c33f-7fac-4f01-a241-ddf4e709106c

[dukenv0307]

Undermining the C+ decision because "my solution is mine and it's better", pinging the internal engineer for that reason: NOT GOOD

@cubuspl42 I'm really sorry if you don't feel good about it 🙏 I didn't mean to undermine your decision, I just meant to contribute to make sure we arrive at the best solution possible.

@dukenv0307 suggests otherwise...

@cubuspl42 yes my solution won't have that gitch. Here's the branch .

[dragnoir]

@cubuspl42 I removed my proposal. after more testing, I don't like the way I did it. I feel it's also a workaround. I will post another proposal if I find a better solution and the "Help wanted" still actif here.

I apologize if my focus on the details caused any inconvenience. I have the utmost respect for your decision and appreciate the opportunity to discuss this matter.

[cubuspl42]

@dragnoir I didn't have any complaints about your attitude, it was more towards @dukenv0307. But we're fine now, everyone seems to agree, let's move on 🙂

I'll proceed with the testing 👍

[cubuspl42]

@aswin-s

I couldn't reproduce the discussed glitch.

But I found another problem:

https://github.com/Expensify/App/assets/2590174/75aa7073-477f-4067-9b79-f1ab2b74084b

Put attention to the spinner. It's not at the center of the preview frame.

@dukenv0307

You use the following style in your code:

!aspectRatio && {opacity: 0}

I thought that when you said:

Also my solution doesn't have the UI glitch

...I understood that your proposal doesn't have this problem, not that you solved it / worked it around (the same way @aswin-s mentioned recently).

[shubham1206agra]

@cubuspl42 Just for sake of completeness, can you test my branch too?

[dukenv0307]

...I understood that your proposal doesn't have this problem, not that you solved it / worked it around (the same way @aswin-s mentioned recently).

@cubuspl42 yes, that is part of my proposal, you can check the part quoted below in my proposal, it was there in my proposal 3 days ago, well before @aswin-s mentioned it, feel free to double check the proposal edit history to confirm it 🙏

For native platforms, we should do similarly in the index.native file, for native platforms, we won't get the image dimensions unless we load the image, we might want to use Image.getSize similar to on web to make sure it's independent from the image loading, and then we can prevent displaying of the image while the aspectRatio is not yet available, it might help prevent some potential issues.

[cubuspl42]

@shubham1206agra I didn't test it, because as I see it, @dukenv0307 is second in line here:

• His solution doesn't have the non-centered spinner problem or other observable bugs
• He was first to notice and suggest a solution/workaround for the non-available aspect ratio (at the time when I didn't understand that this is a serious problem)

Do I miss something objective?

[shubham1206agra]

No I just wanted to know if my solution has some problem. I don't care of selection of proposal.

[aswin-s]

@cubuspl42 The branch that @dukenv0307 shared is not working for wide images, when receipts are stacked. Aspect ratio calculation needs to accommodate images whose height are greater than width and vice versa.

[aswin-s]

@cubuspl42 However, I think these small glitches like these can be refined in the PR stage. I'm wondering, at the proposal stage, should we aim for a completely functional branch, or just propose the overall method for tackling the issue? It might be more efficient to focus on the main idea at this stage. Curious to know your thoughts on this.

[dukenv0307]

@cubuspl42 The branch that @dukenv0307 shared is not working for wide images, when receipts are stacked. Aspect ratio calculation needs to accommodate images whose height are greater than width and vice versa.

@aswin-s yes I know it, the branch is meant to be a POC that my core approach doesn't have the glitch issue. Handling the aspect ratio for case height greater than width is similar to the other way around (just reversed logic) so I think it can be polished in the PR, we already have quite similar logic here.

It might be more efficient to focus on the main idea at this stage.

@aswin-s I agree with you on this 👍

[cubuspl42]

should we aim for a completely functional branch, or just propose the overall method for tackling the issue?

It's a great question. I don't think that there's a good, definitive answer.

If an issue is technical, the major differences among the proposals can lie in details like the ones we discuss. If working around the fact that the spinner is not centered in one solution will require a hack, then choosing a different proposal would very likely be a better option.

When I focus on a main idea when selecting a proposal, there's a 90% chance that I'll get a response from another Contributror saying "the selected proposal has this technical issue X, and my doesn't". And I agree that if that's indeed the case, the other proposal might be a better pick.

It's very easy to sketch an idea that sounds great but doesn't work and fixing its issues requires 3 hacks which later cause 3 regressions. So that's why I sometimes ask about the code which was used to test the idea, as it can contain very ugly make-up above a pretty idea, which is a big regression risk if there are no good ideas for removing these hacks. I don't have to remind you that the compensation system is built around regressions.

It's possible that I focus too much on the details here and overfocus on "fairness". Frankly, most of our proposals here are good enough and could be forged into good PRs with one or two iterations of PR feedback.

[cubuspl42]

Okey, let's wrap this up.

The previously selected proposal by @aswin-s was proven to have issues. It's likely that they could be solved during PR implementation.

At the same time, the proposal by @dukenv0307 also has its strengths. It's solving the problem on a lower level (in-house Image component), which makes the solution more reusable in the future. A risk is that it's also more prone to regressions (as we modify more widely used code), but the new logic is mostly conditional. Also, it's been proven that there are known solutions for related minor technical problems.

We had some interesting meta-discussions about proposal selection and how the technical issues can be solved in the PR implementation phase. While it's difficult to reach ultimate conclusions here, I think we're all left with some notes for the future.

I change my decision toward approving the proposal by @dukenv0307

C+ reviewed 🎀 👀 🎀 (again)

[melvin-bot[bot]]

Current assignee @Beamanator is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.