Skip to content
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.

Sign up for GitHub

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

fixed: Image does not zoom where you click for desktop and web #8809

Merged
merged 9 commits into from
May 17, 2022
Merged

fixed: Image does not zoom where you click for desktop and web #8809

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
80321ec
fixed: Image does not zoom where you click for desktop and web
metehanozyurt Apr 27, 2022
0a1fab4
Changed comments , added meaningful variable names
metehanozyurt May 4, 2022
1c4889d
Merge branch 'main' of https://github.com/Expensify/App into issue-8119
metehanozyurt May 4, 2022
6b53b4a
Merge branch 'main' of https://github.com/Expensify/App into issue-8119
metehanozyurt May 4, 2022
ff8acbb
updated logic comments added
metehanozyurt May 4, 2022
56c8119
correcting a typo
metehanozyurt May 4, 2022
8b38535
unnecesary function removed, better comments added, code has been imp…
metehanozyurt May 14, 2022
485a5fb
Merge branch 'main' of https://github.com/Expensify/App into issue-8119
metehanozyurt May 14, 2022
3a4750b
unnecesary condition removed.
metehanozyurt May 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 43 additions & 78 deletions src/components/ImageView/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,6 @@ class ImageView extends PureComponent {
imgWidth: 0,
imgHeight: 0,
zoomScale: 0,
imageLeft: 0,
imageTop: 0,
imageRight: 0,
imageBottom: 0,
};
}

Expand Down Expand Up @@ -98,17 +94,22 @@ class ImageView extends PureComponent {
* @param {SyntheticEvent} e
*/
onContainerPress(e) {
if (this.state.isZoomed && !this.state.isDragging) {
let sX;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this to something more meaningful?

let sY;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one too

if (this.isZoomed && !this.state.isDragging) {
const {offsetX, offsetY} = e.nativeEvent;
const delta = this.getScrollOffset(offsetX, offsetY);
const sX = delta.offsetX;
const sY = delta.offsetY;
this.scrollableRef.scrollTop = sY * this.state.zoomScale;
this.scrollableRef.scrollLeft = sX * this.state.zoomScale;
const delta = this.getScrollOffset(offsetX / this.state.zoomScale, offsetY / this.state.zoomScale);
sX = delta.offsetX;
sY = delta.offsetY;
}

if (this.state.isZoomed && this.state.isDragging && this.state.isMouseDown) {
if (this.isZoomed && this.state.isDragging && this.state.isMouseDown) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.setState({isDragging: false, isMouseDown: false});
} else if (this.isZoomed) {
this.setState({isZoomed: this.isZoomed}, () => {
this.scrollableRef.scrollTop = sY;
this.scrollableRef.scrollLeft = sX;
});
}
}

Expand All @@ -117,10 +118,17 @@ class ImageView extends PureComponent {
return;
}

this.setState(prevState => ({
isZoomed: !prevState.isZoomed,
isMouseDown: false,
}));
this.isZoomed = !this.state.isZoomed;
if (this.isZoomed === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
        });
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.onContainerPressOut = this.onContainerPressOut.bind(this);

App/src/components/ImageView/index.js

Lines 115 to 124 in 9a8291b

onContainerPressOut() {
if (this.state.isDragging) {
return;
}
this.setState(prevState => ({
isZoomed: !prevState.isZoomed,
isMouseDown: false,
}));
}

App/src/components/ImageView/index.js

Line 287 in 9a8291b

onPressOut={this.onContainerPressOut}

this.setState(prevState => ({
isMouseDown: false,
isZoomed: !prevState.isZoomed,
}));
} else {
this.setState({
isMouseDown: false,
});
}
}

/**
Expand All @@ -129,55 +137,20 @@ class ImageView extends PureComponent {
* @param {Number} imageHeight
*/
setImageRegion(imageWidth, imageHeight) {
let width = imageWidth;
let height = imageHeight;
const containerHeight = this.state.containerHeight;
const containerWidth = this.state.containerWidth;

// return if image not loaded yet
if (imageHeight <= 0 || containerHeight <= 0) {
if (imageHeight <= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need containerHeight check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
}
let imgLeft = (this.props.windowWidth - width) / 2;
let imgRight = ((this.props.windowWidth - width) / 2) + width;
let imgTop = (this.props.windowHeight - height) / 2;
let imgBottom = ((this.props.windowHeight - height) / 2) + height;
const isScreenWiderThanImage = (this.props.windowWidth / width) > 1;
const isScreenTallerThanImage = (this.props.windowHeight / height) > 1;
const aspect = width / height;
if (aspect > 1 && !isScreenWiderThanImage) {
// In case Width fit Screen width and Height not fit the Screen height
const fitRate = this.props.windowWidth / width;
imgLeft = 0;
imgRight = this.props.windowWidth;
imgTop = (this.props.windowHeight - (fitRate * height)) / 2;
imgBottom = imgTop + (fitRate * height);
} else if (aspect <= 1 && !isScreenTallerThanImage) {
// In case Height fit Screen height and Width not fit the Screen width
const fitRate = this.props.windowHeight / height;
imgTop = 0;
imgBottom = this.props.windowHeight;
imgLeft = (this.props.windowWidth - (fitRate * width)) / 2;
imgRight = imgLeft + (fitRate * width);
}

const containerHeight = this.state.containerHeight;
const containerWidth = this.state.containerWidth;
const width = imageWidth;
const height = imageHeight;
const newZoomScale = Math.min(containerWidth / width, containerHeight / height);
this.setState({

this.setState(prevState => ({
imgWidth: width,
zoomScale: newZoomScale,
imgHeight: height,
imageLeft: imgLeft,
imageTop: imgTop,
imageRight: imgRight,
imageBottom: imgBottom,
});
zoomScale: containerHeight ? newZoomScale : prevState.zoomScale,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand the logic here, if containerHeight? In which case will it be 0 (or false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

WhatsApp Image 2022-04-09 at 14 41 10

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}));
}

/**
Expand All @@ -187,29 +160,21 @@ class ImageView extends PureComponent {
* @returns {Object} converted touch point
*/
getScrollOffset(x, y) {
let fitRatio = 1;
if (this.state.imageTop === 0) {
// Fit Height
fitRatio = this.props.windowHeight / this.state.imgHeight;
} else if (this.state.imageLeft === 0) {
// Fit Width
fitRatio = this.props.windowWidth / this.state.imgWidth;
}
let sx = (x - this.state.imageLeft) / fitRatio;
let sy = (y - this.state.imageTop) / fitRatio;
let sx;
let sy;

// White blank touch
if (x < this.state.imageLeft) {
// container size bigger than clicked position offset
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments are title case

if (x <= (this.state.containerWidth / 2)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unnecessary brackets, it should be like so

x <= this.state.containerWidth / 2

sx = 0;
} else if (x > this.state.containerWidth / 2) {
// minus half of container size because we want to be center clicked position
sx = x - (this.state.containerWidth / 2);
}
if (x > this.state.imageRight) {
sx = this.state.imgWidth;
}
if (y < this.state.imageTop) {
if (y <= this.state.containerHeight / 2) {
sy = 0;
}
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments start with Title case

sy = y - (this.state.containerHeight / 2);
}
return {offsetX: sx, offsetY: sy};
}
Expand Down