Fix resizes on image upload #8928

allroundexperts · 2022-05-10T21:25:50Z

Details

Fixed Issues

$ #8590

Tests

In any of the chat room, upload an image via clicking + icon.
The uploaded image in the chat should not resize and remain the same size throughout.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

In any of the chat room, upload an image via clicking + icon.
The uploaded image in the chat should not resize and remain the same size throughout.

Verify that no errors appear in the JS console

Screenshots

Web

167023275-f91c0d23-80bb-4415-9023-d07677fe6544.mp4

Mobile Web

simulator-screen-recording-iphone-13-2022-05-12-at-233355_flQfDWOD.mp4

Desktop

screen-recording-2022-05-12-at-113641-pm_c8BPFcNJ.mp4

iOS

simulator-screen-recording-iphone-13-2022-05-12-at-214353_2o7S3HVD.mp4

Android

screen-recording-2022-05-12-at-112758-pm_odUBRRbl.mp4

github-actions · 2022-05-10T21:26:10Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

allroundexperts · 2022-05-12T19:14:47Z

I have read the CLA Document and I hereby sign the CLA

Santhosh-Sellavel · 2022-05-16T20:16:08Z

Sorry, @allroundexperts missed this one.
Can you pull changes from since this is over a week old?
I'll review it tomorrow!
cc: @chiragsalian

allroundexperts · 2022-05-16T20:22:48Z

@Santhosh-Sellavel Updated.

Santhosh-Sellavel · 2022-05-17T17:15:40Z

@allroundexperts
I am getting a new issue here, the attachment get's hidden for a couple of secs

Attachment.get.hidden.for.a.second.mov

cc: @chiragsalian

src/components/ThumbnailImage.js

allroundexperts · 2022-05-20T16:42:50Z

@Santhosh-Sellavel Is this issue intermittent? Or does it always happen?

Santhosh-Sellavel · 2022-05-20T22:16:34Z

@Santhosh-Sellavel Is this issue intermittent? Or does it always happen?
At that time able to reproduce consistently. The image doesn’t disappear but loading is shown will sending the image.

Screen.Recording.2022-05-21.at.3.43.41.AM.mov

@chiragsalian This is expected behavior right?

chiragsalian · 2022-05-23T18:10:24Z

Hmm so it looks like once we attach an image,

First the image shows up with its own thumbnail with correct dimensions.
Then once Report_AddComment finishes then the loader gif shows up.
Then the loader hides and the same thumbnail image from step 1 shows up.

@allroundexperts, do we need the loader gif? Is it possible to remove it.

The reason i suggest this is because with our offline first design changes if the user is offline we'll still show the attachment thumbail as per step 1 and have a different language to show a comment hasn't been saved in the server. So i think showing the loader in the middle doesn't have much value here. Thoughts?

allroundexperts · 2022-05-23T18:18:05Z

What should we replace it with?

…

On Mon, 23 May 2022 at 11:10 PM, Chirag Chandrakant Salian < ***@***.***> wrote: Hmm so it looks like once we attach an image, 1. First the image shows up with its own thumbnail with correct dimensions. 2. Then once Report_AddComment finishes then the loader gif shows up. 3. Then the loader hides and the same thumbnail image from step 1 shows up. @allroundexperts <https://github.com/allroundexperts>, do we need the loader gif? Is it possible to remove it. [image: image] <https://user-images.githubusercontent.com/14356145/169880937-71529f0d-8d52-452b-a34d-548967a7a86e.png> The reason i suggest this is because with our offline first design changes if the user is offline we'll still show the attachment thumbail as per step 1 and have a different language to show a comment hasn't been saved in the server. So i think showing the loader in the middle doesn't have much value here. Thoughts? — Reply to this email directly, view it on GitHub <#8928 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHFJUUHMXGEVS6CHJPXMYXLVLPCZZANCNFSM5VS4JZIQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

chiragsalian · 2022-05-23T19:31:54Z

Can we not keep it as the attachment thumbnail before the api request was completed? From my comment above the image thumbnail from step 1 and 3 looks identical so i was wondering if we could just not show the loader gif?
If it helps this is what i see,

step 1, i just uploaded the image,
step 2, image uploaded complete,
step 3, loader hides. It looks like the same thumbnail from step 1 is shown,

I was wondering If we could update the code so that the loader from step 2 would not be shown so that the attachment upload looks smoother. I haven't looked at the code closely but i imagine if the response dimensions and image is the same as what we already had then don't show the loader. Thoughts?

allroundexperts · 2022-05-23T19:40:10Z

Can we not keep it as the attachment thumbnail before the api request was completed? From my comment above the image thumbnail from step 1 and 3 looks identical so i was wondering if we could just not show the loader gif? If it helps this is what i see,

step 1, i just uploaded the image,

step 2, image uploaded complete,

step 3, loader hides. It looks like the same thumbnail from step 1 is shown,

I was wondering If we could update the code so that the loader from step 2 would not be shown so that the attachment upload looks smoother. I haven't looked at the code closely but i imagine if the response dimensions and image is the same as what we already had then don't show the loader. Thoughts?

So it's not working like that. As explained in the issue description as well, here's what is happening:

User uploads the image and we have a blob (image stored on users local machine). We display that blob. We also upload that blob to the API. The API call succeeds and we get a response containing the image link on the server with its dimensions.
Next, we download the image from the link (this is where the loader is shown). The placeholder (loading image) is the same size as the thumbnail in step 1 because we have the image dimensions as an API response from the server.
Once the image is downloaded, it is now shown to the user.

The loader is shown while the image is downloaded. It's NOT shown while the API call is being processed.

chiragsalian · 2022-05-23T20:08:36Z

Okay, so let's take your step you mentioned in the earlier comment. Is it possible to to not show the loader while downloading the image when we already have a thumbnail with the proper dimensions? I'm pretty much asking if we can come up with a plan to not show the loader in the middle since it feels unnecessary and weird when the user is sees a proper thumbnail, then sees a loader gif to again see the proper thumbnail from earlier.

Or do you feel there is a time when the user attaches an image that the loader is necessary? If so can you explain why?

I do think when there is no thumbnail the loader should still be shown, like when we switch to a chat and you are downloading messages and images then i think it makes sense to have.

Also if its difficult to change i don't mind keeping it too. Was just wondering if this would be a nice user experience improvement to remove the loader gif when image thumbnail and dimensions are already known.

allroundexperts · 2022-05-23T20:19:25Z

@chiragsalian If you look at the code, you'll see that when the user uploads the image from his local machine, at that point, we create a offline db record and save it in the local storage. That db record contains all the fields as a remote db record would except the image url will be the one that points to the local blob url. As soon as the API request is made, the record that was saved locally on users storage gets replaced with the remote record that has the image url pointing to your storage server.

The approach that you're suggesting would require:
a. Store a duplicate record on local storage which doesn't get over-ridden by the server response.
b. Make the report screen smart enough to choose between the duplicate record and the over-ridden record.

This all won't still work for the other users in the chat room. We'll still need to show them the loader because they only have a single source to get the image from ie your server.

Doing this change for just a single user (other users would still see loader) is not worth the time in my opinion.

Santhosh-Sellavel

Looks Good & Tests well!

cc: @chiragsalian

Santhosh-Sellavel · 2022-05-27T21:46:50Z

PR Reviewer Checklist

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 verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
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 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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- 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 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 */
- Any functional components have the displayName property
- 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 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(themeColors.componentBG)
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.

chiragsalian

Suggested some minor changes but the rest LGTM and works well.

src/components/ThumbnailImage.js

chiragsalian · 2022-06-03T16:52:23Z

Friendly bump @allroundexperts to update the PR based on my review comments.

allroundexperts · 2022-06-03T19:53:34Z

Friendly bump @allroundexperts to update the PR based on my review comments.

Handled.

src/components/ThumbnailImage.js

chiragsalian

LGTM. Thank you for the changes.

PR Reviewer Checklist

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 verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
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 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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- 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 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 */
- Any functional components have the displayName property
- 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 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(themeColors.componentBG)
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.

chiragsalian · 2022-06-03T20:47:41Z

Merging since santhosh approved earlier.

OSBotify · 2022-06-03T20:56:26Z

✋ 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 · 2022-06-06T18:29:29Z

🚀 Deployed to staging by @chiragsalian in version: 1.1.72-0 🚀

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

joelbettner · 2022-06-07T19:51:40Z

@allroundexperts FYI, this got reverted to fix this deploy blocker: #9334

Without taking additional time on a time sensitive issue, we could not narrow down which part of this PR actually caused the bug. It is probably a good idea to "retry" this PR, but make sure we test for the bug from the deploy blocker as the changes are being made so we can pinpoint the issue and fix it.

OSBotify · 2022-06-08T16:30:27Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.73-2 🚀

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

parasharrajat · 2023-12-27T17:14:54Z

src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

+    const imageWidth = htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
+    const imageHeight = htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;


if data-expensify-width is set to "0", it breaks PDF rendering. #33626

allroundexperts added 4 commits May 6, 2022 01:24

fix: incorporated the new image params

646deb1

fix: default props problem

8a95085

fix: more comments

50e38f7

fix: removed stale code

29514a1

allroundexperts marked this pull request as ready for review May 12, 2022 19:10

allroundexperts requested a review from a team as a code owner May 12, 2022 19:10

melvin-bot bot requested review from chiragsalian and Santhosh-Sellavel and removed request for a team May 12, 2022 19:10

allroundexperts changed the title ~~Fix 8590~~ Fix resizes on image upload May 12, 2022

allroundexperts force-pushed the fix-8590 branch from 964a745 to 29514a1 Compare May 12, 2022 19:24

Merge branch 'main' into fix-8590

d004c21

Santhosh-Sellavel suggested changes May 17, 2022

View reviewed changes

src/components/ThumbnailImage.js Outdated Show resolved Hide resolved

src/components/ThumbnailImage.js Outdated Show resolved Hide resolved

fix: handled pr comments

e8ad005

allroundexperts requested a review from Santhosh-Sellavel May 20, 2022 16:52

Merge branch 'Expensify:main' into fix-8590

26dab7c

chiragsalian mentioned this pull request May 23, 2022

[HOLD for payment 2022-06-16] [HOLD for payment 2022-06-15] [$500] Uploading an image causes the thumbnail to resize 3 times #8590

Closed

Refactored Thumbnail Image

268d43a

allroundexperts force-pushed the fix-8590 branch from 1e448a8 to 268d43a Compare May 27, 2022 20:47

allroundexperts requested a review from Santhosh-Sellavel May 27, 2022 20:49

Santhosh-Sellavel previously approved these changes May 27, 2022

View reviewed changes

chiragsalian requested changes May 31, 2022

View reviewed changes

src/components/ThumbnailImage.js Outdated Show resolved Hide resolved

src/components/ThumbnailImage.js Outdated Show resolved Hide resolved

src/components/ThumbnailImage.js Show resolved Hide resolved

Handled comments related to documentation

88bd693

allroundexperts dismissed Santhosh-Sellavel’s stale review via 88bd693 June 3, 2022 19:52

allroundexperts requested a review from chiragsalian June 3, 2022 19:53

Merge branch 'main' into fix-8590

d303e6f

chiragsalian requested changes Jun 3, 2022

View reviewed changes

src/components/ThumbnailImage.js Outdated Show resolved Hide resolved

Removed return type for updateImageSize fn

421f084

allroundexperts requested a review from chiragsalian June 3, 2022 20:34

chiragsalian approved these changes Jun 3, 2022

View reviewed changes

chiragsalian merged commit 50da22f into Expensify:main Jun 3, 2022

yuwenmemon mentioned this pull request Jun 6, 2022

Deploy Checklist: New Expensify 2022-06-06 #9326

Closed

47 tasks

chiragsalian mentioned this pull request Jun 7, 2022

Revert "Fix resizes on image upload" #9343

Merged

amyevans mentioned this pull request Jun 8, 2022

Chat - Preview of the image is not appearing for the first time when it's clicked to open #9333

Closed

parasharrajat reviewed Dec 27, 2023

View reviewed changes

parasharrajat mentioned this pull request Dec 27, 2023

[HOLD for payment 2024-01-04] [HOLD for payment 2024-01-03] [$500] Chat - PDF is not displayed in chat after uploading #33626

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resizes on image upload #8928

Fix resizes on image upload #8928

allroundexperts commented May 10, 2022 •

edited

Loading

github-actions bot commented May 10, 2022 •

edited

Loading

allroundexperts commented May 12, 2022

Santhosh-Sellavel commented May 16, 2022

allroundexperts commented May 16, 2022

Santhosh-Sellavel commented May 17, 2022

allroundexperts commented May 20, 2022

Santhosh-Sellavel commented May 20, 2022

chiragsalian commented May 23, 2022

allroundexperts commented May 23, 2022 via email

chiragsalian commented May 23, 2022

allroundexperts commented May 23, 2022

chiragsalian commented May 23, 2022 •

edited

Loading

allroundexperts commented May 23, 2022

Santhosh-Sellavel left a comment

Santhosh-Sellavel commented May 27, 2022 •

edited

Loading

chiragsalian left a comment

chiragsalian commented Jun 3, 2022

allroundexperts commented Jun 3, 2022

chiragsalian left a comment

chiragsalian commented Jun 3, 2022

OSBotify commented Jun 3, 2022

OSBotify commented Jun 6, 2022

joelbettner commented Jun 7, 2022 •

edited

Loading

OSBotify commented Jun 8, 2022

parasharrajat Dec 27, 2023

		const imageWidth = htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
		const imageHeight = htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;

Fix resizes on image upload #8928

Fix resizes on image upload #8928

Conversation

allroundexperts commented May 10, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented May 10, 2022 • edited Loading

allroundexperts commented May 12, 2022

Santhosh-Sellavel commented May 16, 2022

allroundexperts commented May 16, 2022

Santhosh-Sellavel commented May 17, 2022

allroundexperts commented May 20, 2022

Santhosh-Sellavel commented May 20, 2022

chiragsalian commented May 23, 2022

allroundexperts commented May 23, 2022 via email

chiragsalian commented May 23, 2022

allroundexperts commented May 23, 2022

chiragsalian commented May 23, 2022 • edited Loading

allroundexperts commented May 23, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented May 27, 2022 • edited Loading

PR Reviewer Checklist

chiragsalian left a comment

Choose a reason for hiding this comment

chiragsalian commented Jun 3, 2022

allroundexperts commented Jun 3, 2022

chiragsalian left a comment

Choose a reason for hiding this comment

PR Reviewer Checklist

chiragsalian commented Jun 3, 2022

OSBotify commented Jun 3, 2022

OSBotify commented Jun 6, 2022

joelbettner commented Jun 7, 2022 • edited Loading

OSBotify commented Jun 8, 2022

parasharrajat Dec 27, 2023

Choose a reason for hiding this comment

allroundexperts commented May 10, 2022 •

edited

Loading

github-actions bot commented May 10, 2022 •

edited

Loading

chiragsalian commented May 23, 2022 •

edited

Loading

Santhosh-Sellavel commented May 27, 2022 •

edited

Loading

joelbettner commented Jun 7, 2022 •

edited

Loading