-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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: Profile - No error after uploading corrupted jpg for avatar, console error shows up. #49203
base: main
Are you sure you want to change the base?
Conversation
…sole error shows up. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
}); | ||
}); | ||
return; | ||
} |
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.
@Ollyws, we have to skip corrupt image validation in AttachmentPicker
because, on native devices, we were showing an alert modal if the image was corrupt, which we don't need when using AttachmentPicker
inside AvatarWithImagePicker
because we already perform validation in the AvatarWithImagePicker
component.
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.
Hmm...would it make more sense to do the image validation in AttachmentPicker on both platforms?
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 tried, but I wasn't able to get it working. I'll try again today or tomorrow.
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.
@Ollyws, I tried and we can show the validation modal in AttachmentPicker
for web, but I don't think it's worth it since the native validation shows only the default alert and in this case we should show the our custom modal. We can simply skip the validation in native file. WDYT?
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.
@Ollyws, friendly bump to check the comment above.
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.
We can simply skip the validation in native file
You mean just leave the changes as they are?
Will get to this one tomorrow. |
.then(() => resolve()) | ||
.catch(() => reject(new Error('Error reading file: The file is corrupted'))); | ||
.then((size) => { | ||
if (size.height <= 0 || size.width <= 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.
Why's this change necessary?
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.
On native, when an image is corrupted, ImageSize.getSize
does not throw an error but returns an image with a width and height of -1
.
The same method is used here:
App/src/components/AttachmentPicker/index.native.tsx
Lines 272 to 280 in 37bf6b2
const validateAndCompleteAttachmentSelection = useCallback( | |
(fileData: FileResponse) => { | |
// Check if the file dimensions indicate corruption | |
// The width/height for a corrupted file is -1 on android native and 0 on ios native | |
// We must check only numeric values because the width/height can be undefined for non-image files | |
if ((typeof fileData.width === 'number' && fileData.width <= 0) || (typeof fileData.height === 'number' && fileData.height <= 0)) { | |
showImageCorruptionAlert(); | |
return Promise.resolve(); | |
} |
Sorry for delay, I will work on this today. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Details
Fixed Issues
$ #48707
PROPOSAL: #48707 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4