-
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: allow webp files on newdot #16031
Conversation
@PauloGasparSv @Santhosh-Sellavel 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] |
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.
@PauloGasparSv Are we good with the use of these libraries?
Hey @allroundexperts (and cc @Santhosh-Sellavel) sorry about this, but we had another internal discussion on this (originally to discuss the libs you added) and we decided that:
@allroundexperts would you please remove the change we have now from this P.R. and implement the changes for allowing the I'll change the API logic to allow |
@PauloGasparSv Sure. I'll do that. |
@PauloGasparSv However, I think we should still validate the file types. As an example, consider someone uploading a mp3 file renamed as a |
I agree we should validate it in the client too @allroundexperts! But I think we should go with the decision of the team this time. If this happens again we can start another discussion and mention this issue as a reason to implement the mimeType check (someone will probably find this while investigating if something happens). And it's good thing that the API will throw an error for invalid files even if the extension was changed, it confirms that it's ok to do this changes for now as we are validating the files somewhere. Thks for the help and patience |
Sounds good. I'll revert and add the requested changes. |
4213049
to
039db1f
Compare
Something might be wrong here: However, this is just my guess. I can only test once the API changes are on staging. I've updated the PR with new code though. |
Thks @allroundexperts, the problem was in the API side but that file helped me find it :D I'm putting this on HOLD so we only merge this once the API P.R. is merged too! I already tested this on Web and will test on other platforms and sent the evidence here shortly. |
Reviewer Checklist
Screenshots/VideosFiles I used to test - Download and use to test in iOS and Android |
This is off hold and can be merged :D |
I want make sure there isn't a way around not showing UPDATE: That is happening because of another repo code. This check should render the I'll update that in the |
@allroundexperts would you mind if I put this on HOLD again until the Expensify-Common P.R. Expensify/expensify-common#512 is merged? Then we can pull from main again and update the version of Expensify-Common in |
Sure thing @PauloGasparSv. Why would I mind? 😆 |
The Expensify-Common P.R. Expensify/expensify-common#512 was merged! @allroundexperts can you please update the Expensify-Common version in package.json to the latest commit (similar to here)? - "expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#ce6258b87396d3c471c9b012b9a518e2ee6283c7",
+ "expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#9cf047b9741d3c02820d515dccb9e0a45b6595f5", |
Sure thing! |
@PauloGasparSv Added latest |
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!
All yours @Santhosh-Sellavel
Files I used to test are in a Zip in the bottom of this comment if anyone needs it for testing iOS and Android
Will get it today! |
Reviewer Checklist
Screenshots/VideosWebWebp_web.movMobile Web - ChromeScreen_Recording_20230325_031807_Chrome.mp4Mobile Web - SafarimWeb_webp.mp4DesktopWebp_Desktop.moviOSios-webp.mp4AndroidAndroid_native_webp.mp4 |
@PauloGasparSv I see a few console warnings while adding attachments, this should be related to change but just for logging that we found adding this here, let me know your thoughts. Check between 40th to 60th second in the below video for logs Android_native_webp.mp4 |
Tested it out completely, LGTM mostly. But I ran into a weird case on iOS check the below video. While importing the pics you added for testing on the simulator Actual JPG, GIF, and Webp as JPG are treated as Images and got imported to Photos and webp is treated as doc got copied to files. GIF & Actual JPEG from photos get uploaded successfully. But the webp as JPG is not getting picked via Photos. Actual Webp & Webp as JPG can be uploaded successfully when picked via Files ios-webp.mp4Let me know if need to do something about this @PauloGasparSv
|
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 Mostly just few doubts!
Thks @Santhosh-Sellavel, re-testing this to check that console error/warning!
I think that's ok :D |
Re-tested Web and couldn't see any warnings. I'll test iOS again and merge this if no warnings show up : ) |
All right, couldn't replicate that warning so I'm going to merge this @Santhosh-Sellavel! |
🚀 Deployed to staging by https://github.com/PauloGasparSv in version: 1.2.90-2 🚀
|
Thanks @PauloGasparSv! |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.90-7 🚀
|
Details
This PR adds
webP
as an accepted image format on New Dot.Fixed Issues
$ #15565
PROPOSAL: #15565 (comment)
Tests
Offline tests
N/A
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.Screenshots/Videos
Web
screen-recording-2023-03-24-at-20340-am_8j8o8rXs.mp4
Mobile Web - Chrome
screen-recording-2023-03-24-at-20913-am_yXNcsbbZ.mp4
Mobile Web - Safari
screen-recording-2023-03-24-at-20728-am_gibUlafM.mp4
Desktop
screen-recording-2023-03-24-at-20547-am_rxG3YqES.mp4
iOS
Screen.Recording.2023-03-24.at.2.19.01.AM.mov
Android
Screen.Recording.2023-03-24.at.2.15.12.AM.mov