-
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
Fix receipt image blink when opening it the first time #33860
Fix receipt image blink when opening it the first time #33860
Conversation
@Ollyws please review when you get the chance 🙇 |
@bernhardoj I still seem to be getting a brief translucent flash on Android. Is this also occuring for you? Screen.Recording.2024-01-08.at.16.26.04.mov |
Hmm, somehow after this PR is merged, the image Here is the size comparison before and after the above-linked PR. (the number after the content size object is the zoom scale) If we try to calculate the width/height * zoom scale, we actually get a size that fits the screen. The problem here is that the scale size is animated but the image size is not. So, when we get the new image size, the image is still scaled with the previous zoom scale for a brief moment. In our case, when the image size becomes {height: 5148, width: 2376}, it is still scaled with 0.22031172000665591 instead of 0.14028395846577663. To overcome this new behavior, I'm thinking of a new solution:
Then, in
And we can remove the opacity style from MultiGestureCanvas. With this solution, the image will be shown when the last It works fine so far on my end, can you please test this and let me know whether we should proceed with this new solution or not? Why do I debounce the onLoadEnd and not onLoad? |
@Ollyws let me know your thoughts for the above finding! |
@Ollyws bump |
@bernhardoj Hmmm..debouncing seems like a bit of a work-around, are there any other solutions? |
@Ollyws I'm afraid there is nothing much we can do as this is most likely the issue with expo-image which is actually already out of scope from the initial issue. |
@bernhardoj Ok I suppose we have no choice but to go with the debounce solution then. |
I have no idea.
Will update it tomorrow. |
@Ollyws updated. Please check |
Thanks for the changes. original.movdebounced.movIs there any way we could improve that? |
Yes, that's expected if we use debounce. The image will be delayed at least 500ms (debounce wait time) or a maximum of 1s (max wait). So far, 500ms is a safe value to prevent the multiple onLoad with different sizes being called. One idea to improve it is to have a ref called
This will indicate whether the image size has been cached or not. If not yet, we will debounce it, otherwise we won't debounce it.
What do you think? |
Sounds good to me! |
Updated. Please check |
@marcochavezf Could I get your opinion on this solution. |
It also seems to be causing the perf-tests to fail... |
The reassure is unrelated. It fails on the search page. Happens on |
Friendly bump on #33860 (comment) @marcochavezf |
Still a bit stuck on this one, to be honest I'm not sure the delay on the initial image load is acceptable. Any other ideas @bernhardoj ? |
The second issue doesn't seems reproducible anymore on latest main for me. Can you check that too? You can log the App/src/components/Lightbox/index.tsx Lines 111 to 117 in 668df99
|
@Ollyws bump |
@bernhardoj Looks like it, I'm only getting two calls here, one where the contentSize is undefined and another with the correct dimensions. Does this mean your original solution will work? |
Let me recheck |
@Ollyws yes, I retested a few times and the original solution still work. Please check. |
Looks good thanks! I'll just do a little more testing and hopefully we'll be good to go. |
Reviewer Checklist
Screenshots/VideosAndroid: Native01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: Native03_iOS_Native.mp4iOS: mWeb Safari05_MacOS_Chrome.mp4MacOS: Chrome / Safari06_MacOS_Desktop.mp4MacOS: DesktopiOS_Web.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks guys!
🚀 Deployed to staging by https://github.com/marcochavezf in version: 1.4.48-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.48-0 🚀
|
Details
The receipt image blinks when we open it the first time because the size is not ready yet and we scale it really big.
Fixed Issues
$ #33625
PROPOSAL: #33625 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-01-03.at.13.09.13.mov
Android: mWeb Chrome
Screen.Recording.2024-01-03.at.13.11.56.mov
iOS: Native
Screen.Recording.2024-01-03.at.13.10.20.mov
iOS: mWeb Safari
Screen.Recording.2024-01-03.at.13.11.43.mov
MacOS: Chrome / Safari
Screen.Recording.2024-01-03.at.13.15.13.mov
MacOS: Desktop
Screen.Recording.2024-01-03.at.13.15.35.mov