Fix image file size when uploaded from ios#10297
Conversation
melvin-bot
Bot
requested review from
Santhosh-Sellavel and
mountiny
and removed request for
a team
| // | ||
| // OriginImageRequestHandler.h | ||
| // NewExpensify | ||
| // | ||
| // Created by ntdiary on 2022/8/8. | ||
| // | ||
|
|
||
| #import <React/RCTURLRequestHandler.h> | ||
|
|
||
| @interface OriginImageRequestHandler : NSObject <RCTURLRequestHandler> | ||
|
|
||
| @end |
There was a problem hiding this comment.
This is just a header file created by convention.
| @implementation OriginImageRequestHandler | ||
| { | ||
| NSOperationQueue *_fileQueue; | ||
| } | ||
|
|
||
| RCT_EXPORT_MODULE() | ||
|
|
||
| - (void)invalidate | ||
| { | ||
| [_fileQueue cancelAllOperations]; | ||
| _fileQueue = nil; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is this handler needed?
There was a problem hiding this comment.
in my proposal, the issue root cause is UIImageJPEGRepresentation and UIImagePNGRepresentation.
these functions will change the image binary data when selecting and uploading.
...
for react-native-image-picker, can submit a PR with adding a option to keep origin data.
for react-native, can customize a new handler to deal the image upload.
We need to use this handler to upload the image file instead of the default RCTImageLoader.mm,
because the RCTImageLoader.mm also uses the above two functions.
|
Hi, I thought it was made clear in the proposal 😂
|
|
@mountiny @Santhosh-Sellavel, I'm going to add a short comment. Is this ok ? 🙂 diff --git a/ios/NewExpensify/OriginImageRequestHandler.mm b/ios/NewExpensify/OriginImageRequestHandler.mm
index d6dc968f9..346b43a9e 100644
--- a/ios/NewExpensify/OriginImageRequestHandler.mm
+++ b/ios/NewExpensify/OriginImageRequestHandler.mm
@@ -4,6 +4,8 @@
//
// Created by ntdiary on 2022/8/8.
//
+// Use this handler to upload images returned by react-native-image-picker instead of the default RCTImageLoader.mm
+// See https://github.com/facebook/react-native/issues/33760#issuecomment-1196562161
#import <Foundation/Foundation.h>
#import <ReactCommon/RCTTurboModule.h> |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
@ntdiary I think it would be handy to also explain what the different parts of the file do, what do you think @Santhosh-Sellavel
|
Agree its useful |
|
@mountiny I can try it, and there is something I need to explain 🙂 These two files are copies of ( |
|
How about this revision ? |
|
Can you update the screenshot recording to ensure, everything works as expected? |
|
@mountiny Can you initiate work flows to perform checks |
|
@Santhosh-Sellavel This issue and revision only occurs on ios platform. |
|
I swear to god I pressed that button! I think it would be nice to test this and show video of android working well at least and check the checkboxes in the list too. But we can move ahead even without it this time since it touches ios predominantly (however updating the package could theoretically cause problems to android as well) @ntdiary @Santhosh-Sellavel Great job guys! I hope @ntdiary you will pick up more issues in the repo, it was good to work with you! |
|
You checked this,
But didn't check off the checks that don't apply to the PR. 😂 |
|
@Santhosh-Sellavel uh, sorry, I canceled it first. 😅 |
|
@Santhosh-Sellavel Any chance you could help out with testing on android? I guess once confirmed all works well we can approve and merge 🙌 |
PR Reviewer Checklist
|
|
Hi, I have tested it on android and updated the checklist, although it runs slowly. |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Great job guys, thank you both!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
👋 Just making a note that this introduced a regression with the |
5 participants
Details
react-native-image-pickerFixed Issues
$ #9424
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
https://youtu.be/oQQwplchSw8
Android