-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: Receipt is zoomed in and cropped when rotating the receipt #78497
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
base: main
Are you sure you want to change the base?
Conversation
|
@linhvovan29546 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| // Reset isLoadedRef when source changes to allow onLoad to fire again for new images (e.g., after rotation) | ||
| useEffect(() => { | ||
| isLoadedRef.current = false; | ||
| }, [source]); |
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.
add line break
src/components/Lightbox/index.tsx
Outdated
| useEffect(() => { | ||
| // Clear the content size state to force recalculation of dimensions | ||
| // This ensures that when an image is rotated and gets a new URI, | ||
| // we don't use stale cached dimensions from the previous image | ||
| setInternalContentSize(undefined); | ||
| setLightboxImageLoaded(false); | ||
| setFallbackImageLoaded(false); | ||
| setIsLoading(true); | ||
| // Don't delete from cache here as other components might still need it | ||
| // The new URI will get its own cache entry when loaded | ||
| }, [uri]); |
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.
Can we run this effect only when uri is changed?
This also runs on mount and contentSize is overridden by setInternalContentSize(undefined); so it doesn't get benefit of cached image here:
App/src/components/Lightbox/index.tsx
Line 114 in 9e3ecbb
| const [contentSize, setInternalContentSize] = useState<Dimensions | undefined>(() => cachedImageDimensions.get(uri)); |
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.
Yes, i agree with @aimane-chnaif, this cannot run like this on mount. Other than that, this change looks good to me
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.
@aimane-chnaif I updated.
|
@chrispader does this change make sense to you? Sorry but tagging you since you worked/refactored on image components. This PR is fixing this:
|
@aimane-chnaif i think this change makes sense to me. We just got to make sure to not loose cached image dimensions for other instances of the |
Explanation of Change
fix: Receipt is zoomed in and cropped when rotating the receipt
Fixed Issues
$ #78368
PROPOSAL:
Tests
Offline tests
Same
QA Steps
Same as test
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
Screen.Recording.2025-12-28.at.16.44.06.mov
Android: mWeb Chrome
Screen.Recording.2025-12-28.at.16.50.05.mov
iOS: Native
Screen.Recording.2025-12-28.at.16.48.51.mov
iOS: mWeb Safari
Screen.Recording.2025-12-28.at.16.47.22.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-28.at.16.50.28.mov