Feature: Use attachment gallery everywhere #31308
Conversation
|
@ntdiary Ready for review! 🚀 |
chrispader
force-pushed
the
feat/use-attachment-gallery-everywhere
branch
from
21684a5 to
70c2dd8
Compare
|
Just added a new patch file for This fixes the crash issue when uploading large images. |
|
@ntdiary ready for review and merge 🚀 |
|
@chrispader, great PR, really like the overall changes. It just needs a bit more time for testing some details (might take one day or two). Here's first small thought: could we keep the loading indicator visible until the large image is actually displayed? loading.mp4 |
Sorry, this was actually supposed to work. Gonna fix this any second :) |
Co-authored-by: wentao <[email protected]>
Co-authored-by: wentao <[email protected]>
Co-authored-by: wentao <[email protected]>
Co-authored-by: wentao <[email protected]>
Co-authored-by: wentao <[email protected]>
Co-authored-by: wentao <[email protected]>
There was a problem hiding this comment.
LGTM. Thanks again, @chrispader, great work!
Btw, It's half past midnight now, and I'm starting to feel a bit dizzy, need to go to bed first. 😂
|
I will merge this in a bit, testing it! |
|
@chrispader I just realized the description is missing QA steps, we need them |
just updated it! Is that sufficient? |
|
That's good enough! Merging! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.4.14-0 🚀
|
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.4.14-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.14-6 🚀
|
| isAuthTokenRequired={isAuthTokenRequired} | ||
| encryptedSourceUrl={encryptedSourceUrl} | ||
| isUsedInCarousel={isUsedInCarousel} |
There was a problem hiding this comment.
This line was accidentally removed? This causes regression of switching to another attachment even in zoomed state of PDF.
There was a problem hiding this comment.
Yes you're right. This was removed accidentally.
Is there a PR already? Otherwise i created one here: #33696
| const width = (e.nativeEvent?.width || 0) / PixelRatio.get(); | ||
| const height = (e.nativeEvent?.height || 0) / PixelRatio.get(); |
There was a problem hiding this comment.
@chrispader Can you tell me why this was divided by PixelRatio.get()?
There was a problem hiding this comment.
@shubham1206agra the Pixel ratio multiplication is so that the image has correct pixelation and looks good, if we remove it, the image will look blurry, since the device pixel ratio is high. The issue here is just that the formula we're using is incorrect.
@chrispader can you confirm that this is the intention, and that the formula is wrong as per this
There was a problem hiding this comment.
@dukenv0307 I am not able to see any pixelation if I don't use PixelRatio.get().
@chrispader Do you have a case where I will see the pixelation?
There was a problem hiding this comment.
Maybe Expo Image already took care of this internally.
There was a problem hiding this comment.
@shubham1206agra I see the pixelation very clearly on my iOS simulator
There was a problem hiding this comment.
Yes, this should indeed be * PixelRatio.get() instead.
There was a problem hiding this comment.
Also, it should be added to the fallback image's onLoad function. Please make sure to add it there as well.
| }; | ||
| }) => {scaleX: number; scaleY: number; minScale: number; maxScale: number}; | ||
|
|
||
| const getCanvasFitScale: GetCanvasFitScale = ({canvasSize, contentSize}) => { |
There was a problem hiding this comment.
👋 Coming from #40788
This has introduced an issue on native Android, where if you're viewing a small image, it's scaled up to fill all of the screen
We've resolved this by capping the scaleX and scaleY at 1, so the images are no longer scaled up automatically
There was a problem hiding this comment.
makes sense! thanks for the catch 🙌
@pecanoro @ntdiary @dylanexpensify
Details
This PR adapts the
AttachmentGallery/ImageTransformercomponent so that we can use it instead ofImageZoomwithin theImageViewcomponent.Fixed Issues
$ #28159
PROPOSAL: #28159 (comment)
Tests
Attachment carousel
"Send attachment" preview
Offline tests
Not needed.
QA Steps
Same as in Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
attachment_gallery_android_compressed.mov
Android: mWeb Chrome
Not affected.
iOS: Native
attachment_gallery_compressed.mov
iOS: mWeb Safari
Not affected.
MacOS: Chrome / Safari
Not affected.
MacOS: Desktop
Not affected.