Skip to content
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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Apr 26, 2023
Merged

fix: 16528 upload gif image fluctuate #17454

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8282225
fix: 16528 upload gif image fluctuate
tienifr Apr 14, 2023
db6cfbf
fix: improve code
tienifr Apr 14, 2023
b05be87
fix: remove filename
tienifr Apr 17, 2023
ef16bed
fix: change avatarFileName to originalFileName
tienifr Apr 17, 2023
92b60d7
fix: change returns type
tienifr Apr 18, 2023
ed4470a
fix: merge main
tienifr Apr 21, 2023
40bc8b0
fix: error type
tienifr Apr 21, 2023
2e06be8
Merge branch 'main' into fix/16528-uploading-gif-image-fluctuate
tienifr Apr 24, 2023
63e7599
fix: disable auth token for local image
tienifr Apr 26, 2023
19780e8
fix: merge main
tienifr Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/components/AttachmentModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ class AttachmentModal extends PureComponent {
modalType: CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE,
isConfirmButtonDisabled: false,
confirmButtonFadeAnimation: new Animated.Value(1),
file: props.originalFileName && {
name: props.originalFileName,
},
};

this.submitAndClose = this.submitAndClose.bind(this);
Expand Down
8 changes: 4 additions & 4 deletions src/components/ImageView/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class ImageView extends PureComponent {
this.trackPointerPosition = this.trackPointerPosition.bind(this);

this.state = {
isLoading: false,
isLoading: true,
Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

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?

containerHeight: 0,
containerWidth: 0,
isZoomed: false,
Expand Down Expand Up @@ -275,7 +275,7 @@ class ImageView extends PureComponent {
<Pressable
style={{
...StyleUtils.getZoomSizingStyle(this.state.isZoomed, this.state.imgWidth, this.state.imgHeight, this.state.zoomScale,
this.state.containerHeight, this.state.containerWidth),
this.state.containerHeight, this.state.containerWidth, this.state.isLoading),
...StyleUtils.getZoomCursorStyle(this.state.isZoomed, this.state.isDragging),
...this.state.isZoomed && this.state.zoomScale >= 1 ? styles.pRelative : styles.pAbsolute,
...styles.flex1,
Expand All @@ -285,10 +285,10 @@ class ImageView extends PureComponent {
>
<Image
source={{uri: this.props.url}}
style={this.state.isLoading ? undefined : [
style={[
styles.h100,
styles.w100,
]} // Hide image until finished loading to prevent showing preview with wrong dimensions.
]}
resizeMode={Image.resizeMode.contain}
onLoadStart={this.imageLoadingStart}
onLoad={this.imageLoad}
Expand Down
2 changes: 2 additions & 0 deletions src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,11 +289,13 @@ function updateAvatar(file) {
[currentUserEmail]: {
avatar: file.uri,
avatarThumbnail: file.uri,
avatarFileName: file.name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
avatarFileName: file.name,
originalFileName: file.name,

errorFields: {
avatar: null,
},
pendingFields: {
avatar: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
avatarFileName: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
avatarFileName: null,
originalFileName: null,

},
},
},
Expand Down
1 change: 1 addition & 0 deletions src/pages/DetailsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
originalFileName={details.avatarFileName}
originalFileName={details.originalFileName}

>
{({show}) => (
<PressableWithoutFocus
Expand Down
12 changes: 5 additions & 7 deletions src/styles/StyleUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Contributor

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)}.

Copy link
Contributor Author

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:

App/src/styles/StyleUtils.js

Line 396 in 74d5290

* @param {Number | null} height
.
I don't actually know the best way to do here. What is your thought?

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return {Object | undefined}
* @returns {Object | null}

*/
function getZoomSizingStyle(isZoomed, imgWidth, imgHeight, zoomScale, containerHeight, containerWidth) {
if (imgWidth === 0 || imgHeight === 0) {
return {
height: isZoomed ? '250%' : '100%',
width: isZoomed ? '250%' : '100%',
};
function getZoomSizingStyle(isZoomed, imgWidth, imgHeight, zoomScale, containerHeight, containerWidth, isLoading) {
if (isLoading || imgWidth === 0 || imgHeight === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (isLoading || imgWidth === 0 || imgHeight === 0) {
// Hide image until finished loading to prevent showing preview with wrong dimensions
if (isLoading || imgWidth === 0 || imgHeight === 0) {

return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return undefined;
return null;

Or if we have to return undefined, change jsdoc to * @returns {Object | undefined}

Copy link
Contributor Author

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

}
const top = `${Math.max((containerHeight - imgHeight) / 2, 0)}px`;
const left = `${Math.max((containerWidth - imgWidth) / 2, 0)}px`;
Expand Down