fixed: small images for scaling problem#8761
Conversation
melvin-bot
Bot
requested review from
flodnv and
parasharrajat
and removed request for
a team
| if (imageHeight < containerHeight && imageWidth < containerWidth) { | ||
| imageHeight *= aspectRatio; | ||
| imageWidth *= aspectRatio; | ||
| // imageHeight *= aspectRatio; | ||
| // imageWidth *= aspectRatio; |
There was a problem hiding this comment.
What is the use of this condition now?
There was a problem hiding this comment.
I was undecided about changing this line. You are absolutely right. I immediately delete unnecessary lines of code. Thank you so much @parasharrajat 🙏.
New version look like this.
if ((imageHeight > containerHeight || imageWidth > containerWidth) && imageHeight > imageWidth) {
imageHeight *= aspectRatio;
} else if(imageHeight > containerHeight || imageWidth > containerWidth){
imageWidth *= aspectRatio;
}
|
I am testing it currently. |
|
@metehanozyurt Please correct all the videos. They should
Correct the template of the PR. Add detailed QA steps. |
| const containerWidth = this.state.containerWidth; | ||
|
|
||
| // return if image not loaded yet | ||
| if (imageHeight <= 0 || containerHeight <= 0) { |
There was a problem hiding this comment.
I think this check was precautionary. What will happen if this is not present at line 165.
There was a problem hiding this comment.
Nothing changes you are right again and again . I will remove this line.
There was a problem hiding this comment.
Could you please revert this change? why have you removed || containerHeight <= 0?
| styles.h100, | ||
| ]} | ||
| resizeMode="contain" | ||
| resizeMode={this.state.zoomScale >= 1 ? 'center' : 'contain'} |
There was a problem hiding this comment.
Why do we need to dynamically check and set the resizeMode here? Isn't center will work?
Can you show me videos to indicate the difference it makes?
There was a problem hiding this comment.
You are absolutely right 😮. I focused on fixing the version that works with the previous big image and missed it. This will change a lot of things thank you so much 🙏 . We dont need a lot of changes no more 🙏 🙏 .
Note : To get the zoomScale information, I made changes in componentDidMount and added the View onLayoutand ref callbacks.
| if ((imageHeight > containerHeight || imageWidth > containerWidth) && imageHeight > imageWidth) { | ||
| imageHeight *= aspectRatio; | ||
| imageWidth *= aspectRatio; | ||
| } else if (imageHeight > imageWidth) { | ||
| imageHeight *= aspectRatio; | ||
| } else { | ||
| } else if (imageHeight > containerHeight || imageWidth > containerWidth) { | ||
| imageWidth *= aspectRatio; |
There was a problem hiding this comment.
Please clean up these conditions. Don't you think we should also set the resize mode to center like the web?
There was a problem hiding this comment.
I think we need still contain because when we will use center zoom not working well. same as Web and Desktop version.
if (imageHeight > imageWidth) {
imageHeight *= aspectRatio;
} else {
imageWidth *= aspectRatio;
}I change like this because still need this for bigger images.
otherwise look like this:
resize-mobile.mp4
|
I will all videos take and showing steps (I mixed it up with other videos. I will take more detail). After update the PR steps. Taking videos now. Thank you for your patience, I'm doing my best not to make a mistake again so sorry🙏. |
There was a problem hiding this comment.
Ok, There have been many changes back and forth here. I think it would be great to make a list of things to test and I would ask you run each and test your changes against it.
- A smaller image should show its true dimensions which means no scaling.
- A larger image should be scaled down to the container dimensions respecting the aspect ratio.
- User should be able to zoom on the image via pin-zoom on the mobile web.
- user should be able to zoom on the image via clicking on the area on the image on the web and desktop.
- User should be able to zoom in/out the image on native devices via pinch-zoom.
Other than that, I have questions about why are we using center for resizeMode on the Mobile web and not on the native. And if we don't have to do that on native then how is it working fine?
| // return if image not loaded yet | ||
| if (imageHeight <= 0 || containerHeight <= 0) { | ||
| return; | ||
| } | ||
|
|
||
| // Fit the image to container size if image small than container. | ||
| const aspectRatio = Math.min(containerHeight / imageHeight, containerWidth / imageWidth); | ||
| if (aspectRatio > 1) { | ||
| width *= (aspectRatio); | ||
| height *= (aspectRatio); | ||
| } |
There was a problem hiding this comment.
Why are we removing this code?
There was a problem hiding this comment.
this code scale image to container height (143-147). When image comes smaller than the modal.
There was a problem hiding this comment.
ok, but I think this code is necessary.
if (imageHeight <= 0 || containerHeight <= 0) {
return;
}
There was a problem hiding this comment.
My first offer was as follows.
if (imageHeight <= 0) {
return;
}ContainerHeight may not have arrived when the image is loaded. That's why I suggested a change like this before. In our previous conversations, we also found that sometimes this function is not called first but some times called last . The following codes are actually required for the last calls conditions.
You are right. If the image height is not reached, the continuation of the code does not matter. It won't be able to set the image height anyway.
I will add line like this.
if (imageHeight <= 0) {
return;
}I am making new commit now. Thank you.
The problem mentioned in the issue. small images were scaled. looks like blurry. So this code fix only this problem. If we want to |
|
Ok, Thanks. I was just sharing a list. I will test them myself. |
|
Thank you 🙏 .Please don't get me wrong, I've already take videos and test all of these. This one not working like before. I try in my test videos too but not work. IOS zoom but zoom on page. These are working in test videos too |
|
Sorry for the delay @metehanozyurt. Just need to test it on all platforms. |
|
Oh no problem. Thank you for spending your time on this PR @parasharrajat . |
| const containerWidth = this.state.containerWidth; | ||
|
|
||
| // return if image not loaded yet | ||
| if (imageHeight <= 0 || containerHeight <= 0) { |
There was a problem hiding this comment.
Could you please revert this change? why have you removed || containerHeight <= 0?
parasharrajat
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
I tested this. Overall, looking good except that on native, the thumbnail shown is bigger than the image size which looks odd. Thumbnails can be seen during image loading on the attachment modal.
Use this image.
|
I had concerns because it was called asynchronously, so I thought about the possibility of containerHeight information not coming. But I did a lot of tests and did not encounter a problem. You're right, I will revert this change.
For this situation we cant do anything. I try to use changing resizeMode to I did research about this situation. The best approach is to give a specific width and height and using resizeMode 'contain' when loading. This method has been used already. My suggestion : |
Hmm, I see. Asked here https://expensify.slack.com/archives/C01GTK53T8Q/p1652698911379689 |
|
Thank you so much for your attention @parasharrajat 🙏 . |
|
@metehanozyurt Could you please share the screen recording for both scenarios on the slack?
|
|
https://expensify.slack.com/archives/C01GTK53T8Q/p1652698911379689 Suggests removing the thumbnail preview. @flodnv What are your thoughts for #8761 (review)? |
|
I took a look at that discussion and agree that we should remove the preview. |
|
In fact, this has always been the case. I would like to show it with a photo with a different aspect ratio. This happens to us because the size of the container grows when we zoom. Small size photos just act like big photos now. screen-zoom-sample-Screen-Recording-2022-06-17-at-22.14.11.mp4 |
|
Got it. Thanks. |
| resizeMode={this.state.zoomScale > 1 ? 'center' : 'contain'} // When Image dimensions are lower | ||
| // than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions. | ||
| // Both `center` and `contain` keeps the image centered on both x and y axis. |
There was a problem hiding this comment.
As comments are bigger now, I think it would be better to move the comments before the prop.
e.g.
| resizeMode={this.state.zoomScale > 1 ? 'center' : 'contain'} // When Image dimensions are lower | |
| // than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions. | |
| // Both `center` and `contain` keeps the image centered on both x and y axis. | |
| // When Image dimensions are lower than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions. | |
| // Both `center` and `contain` keeps the image centered on both x and y axis. | |
| resizeMode={this.state.zoomScale > 1 ? 'center' : 'contain'} |
There was a problem hiding this comment.
LGTM. Thanks for the changes @metehanozyurt.
cc: @flodnv
PR Reviewer Checklist
- I verified the PR has a small number of commits behind
main - I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the testing environment is mentioned in the test steps
- I verified testing steps cover success & fail scenarios (if applicable)
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- I verified there are no console errors related to changes in this PR
- I verified proper code patterns were followed (see Reviewing the code)
- I verified comments were added when the code was not self explanatory
- I verified any copy / text shown in the product was added in all
src/languages/*files (if applicable) - I verified proper naming convention for platform-specific files was followed (if applicable)
- I verified style guidelines were followed
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected) - I verified the UI performance was not affected (the performance is the same than
mainbranch) - If a new component is created I verified that a similar component doesn't exist in the codebase
🎀 👀 🎀 C+ reviewed
|
Thanks @parasharrajat @metehanozyurt! Can you please complete the PR checklist and make sure you've done all of it? 🙏 |
|
I can't edit the PR description but I have followed everything on the PR. |
|
I marked some steps in the checklist. But I didn't mark it because I didn't work in some areas( |
|
@parasharrajat @metehanozyurt thanks for your patience. We are discussing internally on how to improve this. @metehanozyurt yes please fill all of them out. If some item does not apply, check it off (I will soon update the template to make this clear) @parasharrajat Can you copy/paste the checklist into a comment of your own here and then complete it? Thanks! |
|
@flodnv Updated. |
flodnv
left a comment
flodnv
left a comment
There was a problem hiding this comment.
@metehanozyurt can you please complete the PR checklist (and check off items that do not apply)?
Also, there are conflicts now 😬
| * @param {Number} imageHeight | ||
| */ | ||
| setScale(containerWidth, containerHeight, imageWidth, imageHeight) { | ||
| if (!containerWidth || !imageWidth) { |
There was a problem hiding this comment.
Sorry, I am asking why this if statement is here? Why are we only checking *Width and not *Height?
|
Good tests! Why do I see the image flicker very quickly in the first second of |
|
I think I checked what I did in the PR checklist. I don't understand the problem or my mistake. |
Expensify needs you to check off ALL items; lots of items are still unchecked. If an item does not apply, check it off.
Let's keep this in the comment thread please: #8761 (comment)
Thanks! 👍 |
|
@metehanozyurt Please merge main. Thanks. |
|
I merged main , uploaded new videos. I have completed the PR checklist. Thank you for your patience @parasharrajat @flodnv 🙏 . |
flodnv
left a comment
flodnv
left a comment
There was a problem hiding this comment.
Thanks! @parasharrajat if you wanna approve one last time, I can merge this!
|
Checking... |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by @chiragsalian in version: 1.1.84-13 🚀
|
4 participants
Details
A picture with a small dimensions should be displayed exactly as it is provided in the preview modal.
Fixed Issues
$ #8471
Tests
1-) Open Expensify app or web
2-) Click an image dimensions lower than the modal.
3-) Notice that it gets not resized small images
4-) Zooming in-out small image
5-) Notice that resize image
6-) Click an image dimensions bigger than the modal.
7-) Notice that it is fit the modal
8-) Zooming in-out big image
9-) Notice that resize image
Note: on mWeb steps (4,5,8,9) not working because
Pressablecomponent not using it.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
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** 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
1-) Open Expensify app or web
2-) Click an image dimensions lower than the modal.
3-) Notice that it gets not resized small images
4-) Zooming in-out small image
5-) Notice that resize image
6-) Click an image dimensions bigger than the modal.
7-) Notice that it is fit the modal
8-) Zooming in-out big image
9-) Notice that resize image
Note: on mWeb steps (4,5,8,9) not working because
Pressablecomponent not using it.Screenshots
Web
web-Screen-Recording-2022-07-06-at-19.52.07.mp4
Mobile Web
ios-web-Screen-Recording-2022-07-06-at-19.50.02.mp4
android-web-Screen-Recording-2022-07-06-at-20.06.04.mp4
Desktop
desktop-Screen-Recording-2022-07-06-at-19.56.57.mp4
iOS
ios-native-Screen-Recording-2022-07-06-at-19.45.29.mp4
Android
android-native-Screen-Recording-2022-07-06-at-20.02.52.mp4