Skip to content
This repository was archived by the owner on Aug 5, 2020. It is now read-only.

Feature canvas scroll positioning#20

Open
mikenikles wants to merge 3 commits intomasterfrom
feature-canvas-scroll-positioning
Open

Feature canvas scroll positioning#20
mikenikles wants to merge 3 commits intomasterfrom
feature-canvas-scroll-positioning

Conversation

@mikenikles
Copy link

Adds the ability to set a default scroll position when an image is zoomed in.

Status: Ready for review

Reviewers: @marlowpayne @jnurse @donnielrt

Changes

  • Added a new canvasScrollPosition options object
  • Specified default values for the above
  • Updated README.md
  • Bumped minor version

Notes

  • The default behaviour is kept by setting both scroll directions to center

@marlowpayne
Copy link

Currently in Magnifik, when a user taps on the thumbnail image, the canvas will open centered relative to where the user tapped on the thumbnail (user taps top left, canvas opens zoomed in to top left).

Does this PR change that behaviour? If it does, is there a way we can keep both use cases?

@mikenikles
Copy link
Author

Thanks @marlowpayne! I've updated the code to reflect that and also made sure that option is enabled by default to keep backwards compatibility.

I also added @donnielrt to get his feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to cache these properties to var horizontal = this.options.canvasScrollPosition.horizontal, and var vertical = this.options.canvasScrollPosition.vertical

@donnielrt
Copy link
Contributor

@mikenikles was this intended to solve a project-specific issue? Curious what the motivation for the change was?

@mikenikles
Copy link
Author

Yes @donnielrt, this PR is a generalized approach to solve a project-specific issue that may be encountered by other users of Magnifik.

@donnielrt
Copy link
Contributor

Awesome, thanks @mikenikles! Like we discussed, let's get a quick round of QA for the change in the project, and we can get this bad boy merged!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants