-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: 16528 upload gif image fluctuate #17454
fix: 16528 upload gif image fluctuate #17454
Conversation
@danieldoglas @eVoloshchak 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] |
@@ -35,7 +35,7 @@ class ImageView extends PureComponent { | |||
this.trackPointerPosition = this.trackPointerPosition.bind(this); | |||
|
|||
this.state = { | |||
isLoading: false, | |||
isLoading: true, |
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.
I want to change this line because it can improve the performance a bit. isLoading is always set to true then false when image is loaded.
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.
Yeah, I agree, it should be set to true initially
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.
Is this ready for testing?
@@ -35,7 +35,7 @@ class ImageView extends PureComponent { | |||
this.trackPointerPosition = this.trackPointerPosition.bind(this); | |||
|
|||
this.state = { | |||
isLoading: false, | |||
isLoading: true, |
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.
Yeah, I agree, it should be set to true initially
src/styles/StyleUtils.js
Outdated
@@ -196,14 +196,12 @@ function getZoomCursorStyle(isZoomed, isDragging) { | |||
* @param {Number} zoomScale | |||
* @param {Number} containerHeight | |||
* @param {Number} containerWidth | |||
* @return {Object} | |||
* @param {Boolean} isLoading | |||
* @return {Object | undefined} |
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.
According to our style guide:
Do not use type unions e.g. {(number|boolean)}.
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.
Thanks @eVoloshchak . I know but I still see other place use type unions:
Line 396 in 74d5290
* @param {Number | null} height |
I don't actually know the best way to do here. What is your thought?
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.
Ok, I think it's acceptable in this case
src/styles/StyleUtils.js
Outdated
@@ -196,14 +196,12 @@ function getZoomCursorStyle(isZoomed, isDragging) { | |||
* @param {Number} zoomScale | |||
* @param {Number} containerHeight | |||
* @param {Number} containerWidth | |||
* @return {Object} | |||
* @param {Boolean} isLoading | |||
* @return {Object | undefined} |
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.
Ok, I think it's acceptable in this case
src/styles/StyleUtils.js
Outdated
@@ -196,14 +196,12 @@ function getZoomCursorStyle(isZoomed, isDragging) { | |||
* @param {Number} zoomScale | |||
* @param {Number} containerHeight | |||
* @param {Number} containerWidth | |||
* @return {Object} | |||
* @param {Boolean} isLoading | |||
* @return {Object | undefined} |
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.
* @return {Object | undefined} | |
* @returns {Object | null} |
width: isZoomed ? '250%' : '100%', | ||
}; | ||
function getZoomSizingStyle(isZoomed, imgWidth, imgHeight, zoomScale, containerHeight, containerWidth, isLoading) { | ||
if (isLoading || imgWidth === 0 || imgHeight === 0) { |
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.
if (isLoading || imgWidth === 0 || imgHeight === 0) { | |
// Hide image until finished loading to prevent showing preview with wrong dimensions | |
if (isLoading || imgWidth === 0 || imgHeight === 0) { |
src/libs/actions/PersonalDetails.js
Outdated
@@ -289,11 +289,13 @@ function updateAvatar(file) { | |||
[currentUserEmail]: { | |||
avatar: file.uri, | |||
avatarThumbnail: file.uri, | |||
avatarFileName: file.name, |
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.
avatarFileName: file.name, | |
originalFileName: file.name, |
src/libs/actions/PersonalDetails.js
Outdated
errorFields: { | ||
avatar: null, | ||
}, | ||
pendingFields: { | ||
avatar: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE, | ||
avatarFileName: null, |
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.
avatarFileName: null, | |
originalFileName: null, |
src/pages/DetailsPage.js
Outdated
@@ -130,6 +130,7 @@ class DetailsPage extends React.PureComponent { | |||
headerTitle={isSMSLogin ? this.props.toLocalPhone(details.displayName) : details.displayName} | |||
source={ReportUtils.getFullSizeAvatar(details.avatar, details.login)} | |||
isAuthTokenRequired | |||
originalFileName={details.avatarFileName} |
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.
originalFileName={details.avatarFileName} | |
originalFileName={details.originalFileName} |
@eVoloshchak Thanks for your review. I've updated my PR, but just one minor question. Why do you suggest change |
It's just easier to read. If that causes problems - I'm perfectly fine with |
@eVoloshchak ok thanks, any updates? |
@eVoloshchak Yes, it's ready for testing |
function getZoomSizingStyle(isZoomed, imgWidth, imgHeight, zoomScale, containerHeight, containerWidth, isLoading) { | ||
// Hide image until finished loading to prevent showing preview with wrong dimensions | ||
if (isLoading || imgWidth === 0 || imgHeight === 0) { | ||
return undefined; |
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.
return undefined; | |
return null; |
Or if we have to return undefined
, change jsdoc to * @returns {Object | undefined}
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.
@eVoloshchak I prefer using undefined. I tested all platforms with undefined and I believe it works well. And changing @returns {Object | undefined}
is good. Thanks
@eVoloshchak I think all comments are resolved, can you help test. Thanks |
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.
This is looking good, tests well on web/Desktop/mWeb
I've encountered a couple of problems in native apps
Android: I'm unable to upload gif as user avatar, get unhandled promise rejection
iOS: getting the following prop type warning when opening user's avatar from chat
Safari mobile: endless loading spinner if you view image after uploading
IMG_0027.MP4
@eVoloshchak Do these bug happens on main? |
actually, we don't need to use the gif, the image is enough |
Can't reproduce the bug with an endless loading spinner in Safari, I think it was resolved in main. But I can verify that propTypes warning is caused by this PR, it's not present in main. |
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.
@tienifr, there is still a propType warning on iOS when opening user's avatar from chat
And please add a screen recording for Android
@eVoloshchak I've updated my PR to fix the console warning. Please help take a look |
@eVoloshchak I see the problem when I record the video for android. The endless loading spinner is shown when I open user's avatar from chat |
Screen.Recording.2023-04-21.at.19.16.14.movThis bug still happens on main |
Can also reproduce this on main. Could you report this in slack, please? |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-04-27.at.00.48.58.movMobile Web - ChromeScreen_Recording_20230426-212828_Chrome.mp4Screen_Recording_20230426-212754_Chrome.mp4Mobile Web - SafariIMG_0040.MP4DesktopScreen.Recording.2023-04-27.at.00.49.51.moviOSIMG_0043.MP4AndroidScreen_Recording_20230427-003613_New.Expensify.mp4Screen_Recording_20230427-003929_New.Expensify.mp4 |
@tienifr, I'm still getting the bug with endless spinner in Safari after uploading a large image Screen.Recording.2023-04-21.at.16.40.46.movHere's how it looks without this PR Screen.Recording.2023-04-21.at.16.39.51.mov |
original-30F2EFCF-D0BA-46DE-8716-FCEC6B11D93F.mp4@eVoloshchak I can't reproduce on physical device. Should we test on physical device? |
I reported the endless spinner bug on android: https://expensify.slack.com/archives/C049HHMV9SM/p1682331646153809 |
I can reproduce it on a physical device. |
@eVoloshchak I found this issue happens even on main Screen.Recording.2023-04-25.at.16.29.46.mp4When I try to upload image on small screen, the api As usual, the uri 2f8f93d#diff-5d4cb2a76afdf22bdefb20480e5a8267efdfb76629f06d2e9562be8ff7c0d618 We add isAuthTokenRequired in |
@eVoloshchak What do you think? Should we handle it on other PR? |
Here is the bug report: https://expensify.slack.com/archives/C049HHMV9SM/p1682425619771099 |
Awesome job investigating this! @danieldoglas, could you chime in on this please?
|
Let's fix it as part of this PR, I'll increase the bounty for the original issue since we're fixing 2 bugs. |
@eVoloshchak I've updated my PR can you help review again? Thanks |
@tienifr, awesome, the issue on mWeb is resolved Screen_Recording_20230426-213041_New.Expensify.mp4 |
@eVoloshchak I think it's the bug on android that I reported before? |
My bad, totally forgot about that, thanks for the reminder |
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.
LGTM!
cc: @danieldoglas
✋ 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/danieldoglas in version: 1.3.7-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.7-3 🚀
|
Details
Fixed Issues
$ #16528
PROPOSAL: #16528 (comment)
Tests
Offline tests
Same as above
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-04-14.at.14.54.59.mp4
Mobile Web - Chrome
RPReplay_Final1681470972.mp4
Mobile Web - Safari
RPReplay_Final1681470324.mp4
Desktop
Screen.Recording.2023-04-14.at.18.21.37.mp4
iOS
Screen-Recording-2023-04-14-at-18.48.32.mp4
Android