-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2022-06-16] [HOLD for payment 2022-06-15] [$500] Uploading an image causes the thumbnail to resize 3 times #8590
Comments
Triggered auto assignment to @iwiznia ( |
Triggered auto assignment to @michaelhaxhiu ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
Triggered auto assignment to @chiragsalian ( |
Cleaned up GH post a little for clarity. |
I hope there's a way we can bring down the number of resize to 0, but here's a quick ProposalI can bring the resizing down to 1 time. This could be done by using the same thumbnail styles as the upload preview thumbnail. i.e Changes will be made in this file. App/src/components/ThumbnailImage.js Lines 35 to 37 in fd0f83a
How it'll look likeScreen.Recording.2022-04-12.at.4.57.34.AM.mov |
This is still pretty janky, can we make it so:
|
Love all those suggestions! |
@rushatgabhane are you cool with those suggestions from @cead22 to update the proposal? Would be great to get going on this one if everyone is aligned! |
@trjExpensify yes, those are good suggestions. I'd love to be proven wrong tho. |
Im unsure if we can get dimensions of an image before its downloaded. With that being said i'm not sure why we show the blank thumbnail. Can that be removed? Besides that i think the rest is fine. Personally i feel like the first thumbnail i.e., the preview thumbnail with loading icon on top should be a smaller thumbnail, with maybe some opacity so that the loading icon is more obvious. But i'll let @cead22 chime in here since he might have more stronger opinions. |
Can't we get the dimensions of the local image? If not, I think 2 & 3 are still worth doing |
Sorry, if you go to the comment above, I say "can we make it so:..." and list those points. I updated the previous comment to make it clearer |
Started a discussion in #expensify-open-source here about further improvements we can make to the flow of image upload (let's not block this job on the discussion though). |
@allroundexperts, unfortunately we had to revert your PR due to this regression issue found by QA. Can you recreate your PR and address that issue as well. |
I'll check that.
…On Wed, Jun 8, 2022 at 12:50 AM Chirag Chandrakant Salian < ***@***.***> wrote:
@allroundexperts <https://github.com/allroundexperts>, unfortunately we
had to revert <#9343> your PR due to
this regression issue <#9334>
found by QA. Can you recreate your PR and address that issue as well.
—
Reply to this email directly, view it on GitHub
<#8590 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHFJUUEYZ56YKCZCFV3OMPDVN6RZDANCNFSM5TCA4OKQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@chiragsalian Created a new PR: |
Nice, thank you for the quick fix. Shall review shortly. |
The new PR was created, reviewed and merged 🙂 |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.73-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-06-15. 🎊 |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.74-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-06-16. 🎊 |
Ok so payment is due today. I'm seeing a regression here and so the payout structure would be:
|
Awaiting confirmation from Chirag for this, as I'm alittle unsure about this regression 👆 |
I don't feel a penalty for this regression is required for C+ because the PR was testing thumbnail loading and not really the attachment modal. So QA finding a regression with attachment modal would have been very hard to catch in the normal PR and hence i don't feel there should be a penalty for the C+. |
Got it, so it seems you are saying this was out of scope of C+ responsibility. Did we make the necessary changes to our process to ensure we don't miss this testing step again? |
I feel like the issue caught by QA was out of the scope of the C+ responsibility since its quite the edge case. Also technically even QA tested and cleared the PR without realizing any issue with it and later found the regression issue. Most likely from their checklist of complete app testing steps.
I'm not sure what can be done differently other than expecting our C+ to do the same regression testing as QA but this list is huge and not worth it for the C+ to go over so im unsure. Maybe we can recommend people to test a bit more beyond the PR's expectation. Not sure but thats a discussion for slack. |
Got it! Thanks for the context. In that case, it seems like the feedback is for QA so they don't miss it next time. Sometimes our regressions are QAs fault, I think that's going to happen from time to time! |
@allroundexperts is paid. @Santhosh-Sellavel can you apply for this upwork job? the OG link expired |
@michaelhaxhiu Applied for the job! |
All are paid. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
Thumbnail should load once uploaded, or if it's a large file it should display a spinner until it's uploaded.
Actual Result:
If you share an image, the uploading thumbnail resizes 3 times. Then it finalizes the upload.
Workaround:
Just wait for the file to upload but it's a jarring experience.
Platform:
Where is this issue occurring?
Version Number: 1.1.52-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
If you click
+
>add attachment
in a DM, and then select this attachment:You'll notice it changes to this momentarily:
And then this:
Upwork job URL: https://www.upwork.com/jobs/~01ca59e5811fecf4d5
Issue reported by: @cead22
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1649352167230159
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: