Change high resolution image with preview #44807

wildan-m · 2024-07-04T06:29:04Z

Details

We have observed significant lag when displaying and interacting with high-resolution images in the modal. This PR aims to modify the behavior when handling high-resolution images.

Fixed Issues

$ #38180
PROPOSAL: #38180 (comment)

Tests

Standard resolution image

Go to any report chat
Select + Icon and select Add Attachment
Upload this image (4000x4000.png)
Verify original image displayed and zoomable
Select Send button
Select successfully uploaded image
Verify original image displayed and zoomable
If there are multiple attachments, verify it can be navigated through arrow buttons and swipe/drag action

Verify that no errors appear in the JS console

High resolution image

Go to any report chat
Select + Icon and select Add Attachment
Upload this image (10000px)
Verify that image name shown with left image icon
Verify that there is info text at the bottom of the image with text: This image is too large to preview before uploading.
Select Send button
Select successfully uploaded image
Verify thumbnail image displayed (1024px version) and zoomable
Verify that there is info text at the bottom of the image with text: This image has been resized for previewing. Download for full resolution.
If there are multiple attachments, ensure they can be navigated using arrow buttons and swipe/drag actions, with the information text appearing or hiding based on the image resolution threshold (5000px)

Verify that no errors appear in the JS console

Offline tests

Standard resolution image

Turn off the network
Go to any report chat
Select + Icon and select Add Attachment
Upload this image (4000x4000.png)
Verify original image displayed and zoomable
Select Send button
The Uploading attachment.. text will appear
Turn on the network
Select successfully uploaded image
Verify original image displayed and zoomable
If there are multiple attachments, verify it can be navigated through arrow buttons and swipe/drag action

Verify that no errors appear in the JS console

High resolution image

Turn off the network
Go to any report chat
Select + Icon and select Add Attachment
Upload this image (10000px)
Verify that only image name shown with left image icon
Select Send button
The Uploading attachment.. text will appear
Turn on the network
Select successfully uploaded image
Verify thumbnail image displayed (1024px version) and zoomable
Verify that there is info text at the bottom of the image with text: This image has been resized for previewing. Download for full resolution.
If there are multiple attachments, ensure they can be navigated using arrow buttons and swipe/drag actions, with the information text appearing or hiding based on the image resolution threshold (5000px)

Verify that no errors appear in the JS console

QA Steps

Same as Test

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Kapture.2024-07-11.at.14.16.01.mp4

Android: mWeb Chrome

Kapture.2024-07-11.at.14.18.48.mp4

iOS: Native

Kapture.2024-07-11.at.14.03.06.mp4

iOS: mWeb Safari

Kapture.2024-07-11.at.14.06.34.mp4

MacOS: Chrome / Safari

Kapture.2024-07-11.at.14.08.55.mp4

MacOS: Desktop

Kapture.2024-07-11.at.14.14.11.mp4

(cherry picked from commit ba6e2be)

…k-to-without-central-pane Fix navigating between CentralPane screens (cherry picked from commit f757005)

(cherry picked from commit f1f472a)

…tion [CP Staging] Fix selection for splits when there is a search term present (cherry picked from commit 47fafdb)

(cherry picked from commit 050641a)

fix: Hold option appears for invoice (cherry picked from commit 6ea15ed)

…havior

…x/38180-change-big-image-with-preview

…image

…x/38180-change-big-image-with-preview

…ay toggling modal footer

wildan-m · 2024-07-04T10:03:10Z

@Expensify/design

melvin-bot · 2024-07-04T10:03:41Z

@dannymcclain @sobitneupane One of you needs to 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]

dubielzyk-expensify · 2024-07-11T03:26:47Z

I think the reason we decided to include this is because the zooming/panning is going to look much much worse on the resized image vs. the high res image the user uploaded. But if you feel like really no one will care we can ditch it. Alternatively we could change it to just say This image has been resized. Download for full resolution.

It's not like we're showing the a 300x300 image though? Isn't just the preview more manageable but still reasonably decent?

I guess I kinda just expect all internet services to process image or resize them. Feels like it'd be weird if Instagram or Slack said something similar which is why I'm calling it out. I guess this warning only shows for very high resolution so maybe it's fine and more transparent.

I trust you know this better than me. Just wanted to add my perspective 😄

…x/38180-change-big-image-with-preview

wildan-m · 2024-07-11T04:40:05Z

@dubielzyk-expensify Consider this case where there are significant differences between width and height. Since the preview image version should maintain its aspect ratio, the compressed image might not be readable even when zoomed in. Without text describing that a higher resolution is available for download, the user might assume that the downloaded version will also be blurry.

Kapture.2024-07-11.at.11.33.08.mp4

dubielzyk-expensify · 2024-07-11T04:56:42Z

Very fair point! Hadn't considered that.

Sorry if I missed this earlier, what's the threshold for when we show this preview warning then? Is it based on the width/height of the image? If so what's the rule?

wildan-m · 2024-07-11T05:03:17Z

@dubielzyk-expensify currently 7000 pixels. If either width or height exceeds the threshold dimension. (ref)

(resolution.width > CONST.IMAGE_HIGH_RESOLUTION_THRESHOLD || resolution.height > CONST.IMAGE_HIGH_RESOLUTION_THRESHOLD)

wildan-m · 2024-07-11T07:24:04Z

@sobitneupane The PR has been updated.

Update video test
Adjust padding for native
Merge main

dannymcclain · 2024-07-11T13:30:14Z

I guess I kinda just expect all internet services to process image or resize them. Feels like it'd be weird if Instagram or Slack said something similar which is why I'm calling it out.

Totally see what you mean, but I think what we have here still makes sense. Slack for example just doesn't show you a preview AT ALL for really big images.

If you want to see the image, you have to download it. We're kinda taking a different approach where we still want you to be able to see something, but I think it's important to let you know that it is not the actual/proper image file, so it's going to be degraded a bit.

I trust you know this better than me. Just wanted to add my perspective 😄

LOL I'm not sure I'd go as far as to say I know better 😂 but I do appreciate you sharing your perspective! Again, I think this treatment still makes sense because it takes a massively large image to even cause this behavior. So I'd imagine most people will never even see this at all.

dubielzyk-expensify · 2024-07-12T00:41:06Z

Makes sense. Thanks for going on the journey. Having seen this example I see the value in the UI given so many of our attachments are likely to be long or PDF like.

sobitneupane

Screenshots/Videos

Android: Native

Screen.Recording.2024-07-12.at.14.45.12.mov

iOS: Native

Screen.Recording.2024-07-12.at.14.32.54.mov

Screen.Recording.2024-07-12.at.14.34.55.mov

iOS: mWeb Safari

Screen.Recording.2024-07-12.at.14.29.09.mov

MacOS: Chrome / Safari

Screen.Recording.2024-07-12.at.14.24.34.mov

Screen.Recording.2024-07-12.at.14.24.53.mov

MacOS: Desktop

Screen.Recording.2024-07-12.at.14.47.24.mov

shawnborton · 2024-07-12T09:17:03Z

Screenshots feel pretty good to me, let's see what the rest of the @Expensify/design team thinks too.

sobitneupane

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

github-actions · 2024-07-12T10:04:16Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
❌ FAILED ❌	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/44807/index.html
The QR code can't be generated, because the android build failed
Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/44807/NewExpensify.dmg	https://44807.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

Julesssss

This is looking good. Lets give @dannymcclain and @dubielzyk-expensify a final chance to review (the test builds can be found above)

dannymcclain · 2024-07-12T13:15:16Z

Looking good to me!

dubielzyk-expensify

LGTM

Julesssss · 2024-07-15T09:13:01Z

Thanks all for your help!

OSBotify · 2024-07-15T09:23:00Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-07-15T21:05:08Z

🚀 Deployed to staging by https://github.com/Julesssss in version: 9.0.7-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-07-15T22:13:52Z

🚀 Cherry-picked to staging by https://github.com/thienlnam in version: 9.0.7-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2024-07-17T00:20:28Z

🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2024-07-18T13:50:38Z

🚀 Deployed to production by https://github.com/mountiny in version: 9.0.8-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify and others added 23 commits June 26, 2024 20:46

Update version to 9.0.2-1

751f540

(cherry picked from commit ba6e2be)

Merge pull request Expensify#44444 from software-mansion-labs/fix/lin…

d7542be

…k-to-without-central-pane Fix navigating between CentralPane screens (cherry picked from commit f757005)

Update version to 9.0.2-2

98e5373

(cherry picked from commit f1f472a)

Merge pull request Expensify#44505 from Expensify/yuwen-fixSplitSelec…

20f13e8

…tion [CP Staging] Fix selection for splits when there is a search term present (cherry picked from commit 47fafdb)

Update version to 9.0.2-3

d6c5cc5

(cherry picked from commit 050641a)

Merge pull request Expensify#44508 from etCoderDysto/fixHold

75d4ce9

fix: Hold option appears for invoice (cherry picked from commit 6ea15ed)

Change big image preview to thumbnail

7076b54

Move hi Res image caption to attachment modal, change local upload be…

012de0a

…havior

fix isHiResImage logic

8832aad

change isUsedInAttachmentModal to isUploaded for more accurate result

26d983a

Add info icon to hi res image text

3cdd20b

Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…

aa6b459

…x/38180-change-big-image-with-preview

Refactor, remove unnecessary code

04472fe

Remove redundant log, resolve typescript error

f48e435

Add attachmentImageResized localization

440fa48

prettier, refactor

baf2452

replace rn text to custom text, remove unnecessary code, fix lint error.

79bcfb5

Fix bug multiple attachments

72e0409

Fix crash on ios when tapping arrow on hi res to standard resolution …

a8d61d9

…image

Run prettier, fix lint

6dafdc6

Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…

c67aa8a

…x/38180-change-big-image-with-preview

lint

1acae72

Add comment to explain why using isAttachmentCarouselScrolling to del…

8ca81d8

…ay toggling modal footer

wildan-m marked this pull request as ready for review July 4, 2024 10:03

wildan-m requested a review from a team as a code owner July 4, 2024 10:03

melvin-bot bot requested review from dannymcclain and sobitneupane and removed request for a team July 4, 2024 10:03

wildan-m added 4 commits July 11, 2024 10:57

Adjust high resolution info padding

c34b47f

Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…

3534200

…x/38180-change-big-image-with-preview

run prettier

bba81bd

fix isHighResolutionImage

78ac528

re-adjust vertical padding highResolutionInfoPadding

4c23e80

fix highresolutioninfo padding

fa37c50

sobitneupane reviewed Jul 12, 2024

View reviewed changes

sobitneupane approved these changes Jul 12, 2024

View reviewed changes

melvin-bot bot requested a review from Julesssss July 12, 2024 09:28

Julesssss approved these changes Jul 12, 2024

View reviewed changes

dubielzyk-expensify approved these changes Jul 15, 2024

View reviewed changes

Julesssss approved these changes Jul 15, 2024

View reviewed changes

Julesssss merged commit 75e3e2f into Expensify:main Jul 15, 2024
18 of 19 checks passed

github-actions bot mentioned this pull request Jul 15, 2024

Deploy Checklist: New Expensify 2024-07-15 #45421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change high resolution image with preview #44807

Change high resolution image with preview #44807

wildan-m commented Jul 4, 2024 •

edited

Loading

wildan-m commented Jul 4, 2024

melvin-bot bot commented Jul 4, 2024

dubielzyk-expensify commented Jul 11, 2024

wildan-m commented Jul 11, 2024

dubielzyk-expensify commented Jul 11, 2024 •

edited

Loading

wildan-m commented Jul 11, 2024 •

edited

Loading

wildan-m commented Jul 11, 2024

dannymcclain commented Jul 11, 2024

dubielzyk-expensify commented Jul 12, 2024

sobitneupane left a comment

shawnborton commented Jul 12, 2024

sobitneupane left a comment

github-actions bot commented Jul 12, 2024

Julesssss left a comment

dannymcclain commented Jul 12, 2024

dubielzyk-expensify left a comment

Julesssss commented Jul 15, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 17, 2024

OSBotify commented Jul 18, 2024

Change high resolution image with preview #44807

Change high resolution image with preview #44807

Conversation

wildan-m commented Jul 4, 2024 • edited Loading

Details

Fixed Issues

Tests

Standard resolution image

High resolution image

Offline tests

Standard resolution image

High resolution image

QA Steps

PR Author Checklist

Screenshots/Videos

wildan-m commented Jul 4, 2024

melvin-bot bot commented Jul 4, 2024

dubielzyk-expensify commented Jul 11, 2024

wildan-m commented Jul 11, 2024

dubielzyk-expensify commented Jul 11, 2024 • edited Loading

wildan-m commented Jul 11, 2024 • edited Loading

wildan-m commented Jul 11, 2024

dannymcclain commented Jul 11, 2024

dubielzyk-expensify commented Jul 12, 2024

sobitneupane left a comment

Choose a reason for hiding this comment

Screenshots/Videos

shawnborton commented Jul 12, 2024

sobitneupane left a comment

Choose a reason for hiding this comment

Reviewer Checklist

github-actions bot commented Jul 12, 2024

Julesssss left a comment

Choose a reason for hiding this comment

dannymcclain commented Jul 12, 2024

dubielzyk-expensify left a comment

Choose a reason for hiding this comment

Julesssss commented Jul 15, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 15, 2024

OSBotify commented Jul 17, 2024

OSBotify commented Jul 18, 2024

wildan-m commented Jul 4, 2024 •

edited

Loading

dubielzyk-expensify commented Jul 11, 2024 •

edited

Loading

wildan-m commented Jul 11, 2024 •

edited

Loading