Fix: Blurred images in attachment view on mobile devices #6442
Conversation
| @@ -102,21 +144,14 @@ class ImageView extends PureComponent { | |||
| this.imageZoomScale = scale; | |||
| }} | |||
| > | |||
| <ImageWithSizeCalculation | |||
There was a problem hiding this comment.
hmmm I wonder if we should move all this logic to this component instead... what's the value of keeping both?
There was a problem hiding this comment.
ImageWithSizeCalculation component is used in Web & Desktop platforms as well and current implementation works just fine for those. Keeping the logic contained in ImageView native implementation, we basically keep the unnecessary dependencies out of web bundle & avoid webpack errors. If you still prefer to keep it in ImageSizeWithCalculation then it needs to have a native implementation which uses FastImage & RNImageSize.
There was a problem hiding this comment.
Oh ok ok, no, this is good.
|
Argh damn, sorry you got conflicts. |
|
@iwiznia Resolved conflicts |
| } | ||
|
|
||
| componentWillUnmount() { | ||
| if (!this.state.interactionPromise) { return; } |
There was a problem hiding this comment.
I think our style guide says to not do this (ie: we should have return in a new line and the close bracket in another one.
Having said that, you can change this to: this.state.interactionPromise && this.state.interactionPromise.cancel()
There was a problem hiding this comment.
Oh that actually throws Expected an assignment or function call and instead saw an expression.eslintno-unused-expressions lint error.
There was a problem hiding this comment.
I changed it to return in new line.
|
Just realized I had to approve you as contributor for the workflow to run. Please ping me again to merge it when the workflow runs successfully. |
|
@iwiznia All checks passed. |
|
Awesome, can you clarify in the QA steps to do the tests with images of various sizes, especially with big image sizes? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
As stated over here, I'm going to revert this PR because it caused a regression. |
|
Darn, couldn't be reverted automatically 😭 |
|
🚀 Deployed to production by @roryabraham in version: 1.1.17-7 🚀
|
|
This PR is party responsible for a regression here #15288 The |
| // Wait till animations are over to prevent stutter in navigation animation | ||
| this.state.interactionPromise = InteractionManager.runAfterInteractions(() => this.calculateImageSize()); |
There was a problem hiding this comment.
@aswin-s Do you recall the production steps for the animation bug here? We are looking to remove InteractionManager.runAfterInteractions.
There was a problem hiding this comment.
Wow this was over 2 years ago. If I remember correctly trying to calculate image size while animating the modal was causing frozen frames on android device. But this component has gone through multiple refactors after this change and even react-native-fast-image was forked to improve load performance. So this optimisation might not be required anymore.
Details
This PR fixes issue an issue with image attachment view, where the images were blurred out once loaded.
Fixed Issues
$ #5193
Tests
QA Steps
Tested On
Screenshots
Issue is present only on mobile devices.
iOS
Simulator.Screen.Recording.-.iPhone.12.-.2021-11-24.at.02.21.49.mp4
Android
Screen.Recording.2021-11-24.at.2.25.47.AM.mp4