fixed: Image does not zoom where you click for desktop and web #8809
Conversation
melvin-bot
bot
requested review from
chiragsalian and
mananjadhav
and removed request for
a team
src/components/ImageView/index.js
Outdated
| if (y > this.state.imageBottom) { | ||
| sy = this.state.imgHeight; | ||
| } else if (y > this.state.containerHeight / 2) { | ||
| // minus half of container size because we want to be center clicked position |
There was a problem hiding this comment.
Comments start with Title case
src/components/ImageView/index.js
Outdated
| @@ -98,17 +94,22 @@ class ImageView extends PureComponent { | |||
| * @param {SyntheticEvent} e | |||
| */ | |||
| onContainerPress(e) { | |||
| if (this.state.isZoomed && !this.state.isDragging) { | |||
| let sX; | |||
There was a problem hiding this comment.
Can we rename this to something more meaningful?
src/components/ImageView/index.js
Outdated
| @@ -98,17 +94,22 @@ class ImageView extends PureComponent { | |||
| * @param {SyntheticEvent} e | |||
| */ | |||
| onContainerPress(e) { | |||
| if (this.state.isZoomed && !this.state.isDragging) { | |||
| let sX; | |||
| let sY; | |||
src/components/ImageView/index.js
Outdated
| } | ||
|
|
||
| if (this.state.isZoomed && this.state.isDragging && this.state.isMouseDown) { | ||
| if (this.isZoomed && this.state.isDragging && this.state.isMouseDown) { |
There was a problem hiding this comment.
We removed this from state because? and I can still see this.state.isZoomed used. Can you elaborate on why the class level will also required as well as the state?
There was a problem hiding this comment.
We need to use it state.isZoomed to detect the change in rendering. I set the class level because when the state changes, the information of the clicked point is incorrect on containerPress . We hold state change , make calculations then set isZoomed state.
I know it looks bad. But if we let the state change, we were losing the clicked coordinates.
|
|
||
| // return if image not loaded yet | ||
| if (imageHeight <= 0 || containerHeight <= 0) { | ||
| if (imageHeight <= 0) { |
There was a problem hiding this comment.
We don't need containerHeight check?
There was a problem hiding this comment.
As I mentioned in the comment below (#8809 (comment)) , it can sometimes be called last. therefore it is still necessary for the calculations we use below.
src/components/ImageView/index.js
Outdated
| imageRight: imgRight, | ||
| imageBottom: imgBottom, | ||
| }); | ||
| zoomScale: containerHeight ? newZoomScale : prevState.zoomScale, |
There was a problem hiding this comment.
I didn't understand the logic here, if containerHeight? In which case will it be 0 (or false)?
There was a problem hiding this comment.
I'm a little obsessed with this comment (#8119 (comment)) and i found the reason.
Usually setImageRegion calls first and we dont need to set zoomScale we just set the imgWidth and imgHeight. But sometimes the call order can be change and setImageRegion function calls last so we have last chance to set zoomScale. Here video and picture:
162569484-d35970dd-e961-4acf-b215-40ae5b329fe4.mov
There was a problem hiding this comment.
i'm not sure how to reproduce but i don't mind keeping this. But let's add a comment explaining why its needed so that someone in future does not easily remove this by accident.
src/components/ImageView/index.js
Outdated
|
|
||
| // White blank touch | ||
| if (x < this.state.imageLeft) { | ||
| // container size bigger than clicked position offset |
There was a problem hiding this comment.
Comments are title case
|
@metehanozyurt Ping me once you need me to take a look again. Few more comments:
Once all comments are resolved, I'll take a look again and test it locally at my end. |
|
Thank you @mananjadhav comments. I think I'm ready for you 😇. Can you please take a look, thank you. If you'll excuse me, I have a question. The PR I prepared solves the problem mentioned in the issue (#8119). But there was no such support for mWeb before. For this reason, it is the principle of working as it was before. Is this a problem? |
Okay I'll go through this and get back.
IMO, it shouldn't be a problem. @chiragsalian can comment once. But quick question, are you aware of why isn't it working? Lack of platform support? Incorrect code, etc.? |
|
Thank you. If it helps this code works for mWeb. I noticed it while testing for other platforms. When I took the code back and looked again, I observed the same situation. I guess it is not preferred for pinch effect but pinch not working too. App/src/components/ImageView/index.js Lines 243 to 265 in 9a8291b |
|
Yeah its not a problem me too if you don't focus on mWeb for this. If there is a simple fix for mWeb that would be cool but you don't need to focus on it and we can open up another issue for mWeb if we feel its worth it. |
|
Any update here. @mananjadhav, i believe its waiting for a review? |
|
Will test tonight |
src/components/ImageView/index.js
Outdated
| this.scrollableRef.scrollLeft = sX * this.state.zoomScale; | ||
|
|
||
| // We divide the clicked positions by the zoomScale. | ||
| // We need pixel coordinates. |
There was a problem hiding this comment.
| // We need pixel coordinates. | |
| // Divide the clicked position by the zoomScale to get the pixel coordinates |
There was a problem hiding this comment.
Once you accept this suggestion remember to remove the first line of the comment otherwise it will be a dupe.
There was a problem hiding this comment.
Also comments should generally explain "why" its needed not "what" it does. The "what" part is usually self explanatory from the code, because i see delta is being set by dividing offsets i.e., the clicked positions with zoomscale, having the comment explain the same is not valuable.
I would say to either remove it or rephrase it like so,
// Dividing clicked positions by the zoom scale to get coordinates so that once we zoom we will scroll to the clicked location.
|
PR Reviewed. @chiragsalian Looks good to me. Thanks, @metehanozyurt for the patience. It was a big change and I needed to be sure that it doesn't break anything. @metehanozyurt 1 minor comment and please merge the latest #### PR Reviewer Checklist
|
There was a problem hiding this comment.
Works well, left a few comments. Additionally in your test steps you mention,
14-) Click any dimension image on ios and android native apps notice that pinch and pan works. For Android need too double top image first then pinch works like on test video.
15-) for mWeb pinch and double click not work. For ios pinch zooming modal only.
For iOS native i also needed to double click to zoom into image, and it only can zoom to center and not a specific area. While i think thats fine i wasn't sure if you test steps mentioned the same for iOS or if the expectation was different. Can you make it more clear?
PR reviewer checklist
- 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 steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- 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 (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all
src/languages/*files - I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - Any functional components have the
displayNameproperty - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
- If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
src/components/ImageView/index.js
Outdated
| this.scrollableRef.scrollLeft = sX * this.state.zoomScale; | ||
|
|
||
| // We divide the clicked positions by the zoomScale. | ||
| // We need pixel coordinates. |
There was a problem hiding this comment.
Once you accept this suggestion remember to remove the first line of the comment otherwise it will be a dupe.
src/components/ImageView/index.js
Outdated
| this.scrollableRef.scrollLeft = sX * this.state.zoomScale; | ||
|
|
||
| // We divide the clicked positions by the zoomScale. | ||
| // We need pixel coordinates. |
There was a problem hiding this comment.
Also comments should generally explain "why" its needed not "what" it does. The "what" part is usually self explanatory from the code, because i see delta is being set by dividing offsets i.e., the clicked positions with zoomscale, having the comment explain the same is not valuable.
I would say to either remove it or rephrase it like so,
// Dividing clicked positions by the zoom scale to get coordinates so that once we zoom we will scroll to the clicked location.
src/components/ImageView/index.js
Outdated
| this.setState({isDragging: false, isMouseDown: false}); | ||
| } else if (this.isZoomed) { | ||
| // We set isZoomed state then scroll image for calculating positions |
There was a problem hiding this comment.
Not sure why "for calculating positions" is mentioned here. I think the comment will be better read like so,
// We first zoom and once its done then we scroll to the location the user clicked
Thoughts?
src/components/ImageView/index.js
Outdated
| isMouseDown: false, | ||
| })); | ||
| // We hold for set isZoomed state if true | ||
| // Because when set isZoomed state can't getting actual position user clicked |
There was a problem hiding this comment.
I don't follow your comments here. Can you rephrase? I think you are trying to say onContainerPress calculates where to scroll so we zoom in over here and not here, is that correct?
If so how does this rephrase sound?
// We won't set isZoomed true in the block below and instead set it in onContainerPress as that method calculates the scroll positions after zooming.
src/components/ImageView/index.js
Outdated
| // We hold for set isZoomed state if true | ||
| // Because when set isZoomed state can't getting actual position user clicked | ||
| this.isZoomed = !this.state.isZoomed; | ||
| if (this.isZoomed === false) { |
There was a problem hiding this comment.
I'm a little confused why this logic is in onContainerPressOut and why this.isZoomed is needed.
I feel like this can be simplified if it lived in onContainerPress, then we wouldn't need this.isZoomed or even need to do multiple setStates.
So wouldn't this onContainerPress code work? (untested)
onContainerPress(e) {
let scrollX;
let scrollY;
if (!this.state.isZoomed && !this.state.isDragging) {
const {offsetX, offsetY} = e.nativeEvent;
// We divide the clicked positions by the zoomScale.
// We need pixel coordinates.
const delta = this.getScrollOffset(offsetX / this.state.zoomScale, offsetY / this.state.zoomScale);
scrollX = delta.offsetX;
scrollY = delta.offsetY;
}
if (this.state.isZoomed && this.state.isDragging && this.state.isMouseDown) {
this.setState({isDragging: false, isMouseDown: false});
} else {
// We first zoom and once its done if we are zooming in then we scroll to the location the user clicked.
this.setState(prevState => ({
isZoomed: !prevState.isZoomed,
isMouseDown: false,
}), () => {
this.scrollableRef.scrollTop = scrollY;
this.scrollableRef.scrollLeft = scrollX;
});
}
}There was a problem hiding this comment.
It worked perfectly. I wish I could think of that. Thank you so much. Here is the test video. I will use this code and re-shoot videos for web and desktop.
onContainerPress-2022-05-14-at-01.07.04_1.mp4
Thanks to this update, we can delete these lines.
App/src/components/ImageView/index.js
Line 26 in 9a8291b
App/src/components/ImageView/index.js
Lines 115 to 124 in 9a8291b
App/src/components/ImageView/index.js
Line 287 in 9a8291b
src/components/ImageView/index.js
Outdated
| imageRight: imgRight, | ||
| imageBottom: imgBottom, | ||
| }); | ||
| zoomScale: containerHeight ? newZoomScale : prevState.zoomScale, |
There was a problem hiding this comment.
i'm not sure how to reproduce but i don't mind keeping this. But let's add a comment explaining why its needed so that someone in future does not easily remove this by accident.
src/components/ImageView/index.js
Outdated
| if (y < this.state.imageTop) { | ||
| sy = 0; | ||
| // Container size bigger than clicked position offset | ||
| if (x <= (this.state.containerWidth / 2)) { |
There was a problem hiding this comment.
remove unnecessary brackets, it should be like so
x <= this.state.containerWidth / 2
|
Thank you for your valuable comments and patience @mananjadhav and @chiragsalian. I will study harder to give better explanations. You are right about this line. I've been paranoid. I took it back as it was an unnecessary condition. zoomScale: containerHeight ? newZoomScale : prevState.zoomScale,I shot new videos for desktop and web. It works as it should, after new changes. I hope you like it, thank you very much again 🙏 . |
|
Thanks for the changes @metehanozyurt. Appreciate your patience. I will test this again tonight. Thanks, @chiragsalian for the suggestions. |
There was a problem hiding this comment.
Nice, this looks much cleaner. Thank you for the changes @metehanozyurt.
PR Reviewer Checklist
- 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 steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- 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 (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all
src/languages/*files - I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - Any functional components have the
displayNameproperty - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
- If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Thank you so much for helps and patience @chiragsalian , @mananjadhav 🙏 . |
|
🚀 Deployed to staging by @chiragsalian in version: 1.1.62-0 🚀
|
|
🚀 Deployed to production by @AndrewGable in version: 1.1.62-0 🚀
|
Details
For desktop and web versions big images can zoom clicked area problem solved.
Fixed Issues
$ #8119
Tests
Test Files:
testFile1
testFile2
testFile3
testFile4
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
Test Files:
testFile1
testFile2
testFile3
testFile4
Screenshots
Web
onContainerPress-2022-05-14-at-01.07.04_1.mp4
corner click video:
web-corners-click.mp4
Mobile Web
ios-safari.mp4
android-web.mp4
Desktop
Desktop-Screen-Recording-2022-05-14-at-10.13.32.mp4
corner click video:
desktop-corner-click_1.mp4
iOS
ios-native.mp4
Android
android-native.mp4