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.

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(zoom-pane): set min and max values correctly when image is smaller than container #69

Merged
merged 3 commits into from
Jul 9, 2018

Conversation

frederickfogerty
Copy link
Contributor

Description

In the past there was an issue where if the zoomed image's width was smaller than the width of the zoomed image's container, the image would jump around the container when panning around the image. This is shown here #65.

This PR updates the positioning logic of the zoom pane to ensure that the image is rendered correctly in both situations (when the image is smaller than the container, and when it is bigger).

  • when image is smaller: the image is centred in the container
  • when image is larger: behaviour is unchanged

Bug Fix

  • All existing unit tests are still passing (if applicable)
  • [N/A] Add new passing unit tests to cover the code introduced by your PR
  • [N/A] Update the readme
  • [N/A] Update or add any necessary API documentation
  • Add some steps so we can test your bug fix
  • The PR title is in the conventional commit format: e.g. fix(<area>): fixed bug #issue-number
  • Add your info to the contributors array in package.json!

Steps to Test

driftfixed

@frederickfogerty frederickfogerty requested a review from jayeb July 9, 2018 00:45
@@ -94,31 +115,21 @@ export default class ZoomPane {
let scrollY = window.pageYOffset;

let inlineLeft =
triggerRect.left +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes are just performance changes, removing calls to this.el.clientWidth (or similar) and replacing them with elWidth, which reduces the number of DOM calls, improving performance

: differenceBetweenContainerWidthAndImgWidth;
const maxTop = isContainerLargerThanImgY
? differenceBetweenContainerHeightAndImgHeight / 2
: differenceBetweenContainerHeightAndImgHeight;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These previous lines (99-107) are the only functional changes in this PR, along with updating the conditionals on lines 130 and 136 to use these values

Copy link
Contributor

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the ultra-descriptive variables names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants