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.

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
7 changes: 7 additions & 0 deletions src/components/AttachmentModal.js
Original file line number Diff line number Diff line change
@@ -56,6 +56,9 @@ const propTypes = {
/** The ID of the report that has this attachment */
reportID: PropTypes.string,

/** Filename of the attachment */
fileName: PropTypes.string,

...withLocalizePropTypes,

...windowDimensionsPropTypes,
@@ -65,6 +68,7 @@ const defaultProps = {
source: '',
onConfirm: null,
originalFileName: '',
fileName: '',
isAuthTokenRequired: false,
allowDownload: false,
headerTitle: null,
@@ -86,6 +90,9 @@ class AttachmentModal extends PureComponent {
modalType: CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE,
isConfirmButtonDisabled: false,
confirmButtonFadeAnimation: new Animated.Value(1),
file: props.fileName ? {
tienifr marked this conversation as resolved.
Show resolved Hide resolved
name: props.fileName,
} : undefined,
};

this.submitAndClose = this.submitAndClose.bind(this);
8 changes: 4 additions & 4 deletions src/components/ImageView/index.js
Original file line number Diff line number Diff line change
@@ -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,
@@ -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,
@@ -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}
2 changes: 2 additions & 0 deletions src/libs/actions/PersonalDetails.js
Original file line number Diff line number Diff line change
@@ -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,

},
},
},
1 change: 1 addition & 0 deletions src/pages/DetailsPage.js
Original file line number Diff line number Diff line change
@@ -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
fileName={details.avatarFileName}
tienifr marked this conversation as resolved.
Show resolved Hide resolved
>
{({show}) => (
<PressableWithoutFocus
12 changes: 5 additions & 7 deletions src/styles/StyleUtils.js
Original file line number Diff line number Diff line change
@@ -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:

* @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`;