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

Recalculate original state when resize() was called #188

Merged
merged 1 commit into from
Nov 21, 2016
Merged

Recalculate original state when resize() was called #188

merged 1 commit into from
Nov 21, 2016

Conversation

kevinlaw91
Copy link
Contributor

Proposed fix for #185

@kevinlaw91
Copy link
Contributor Author

Might need to refactor the fix because most of the code in this fix was borrowed from ShadowViewport.prototype.processCTM()

We cannot reuse processCTM() in the fix because we don't want to call setCTM() at the end. Calling it will reset zoom and pan every time when viewport resizes.

@bumbu
Copy link
Owner

bumbu commented Mar 27, 2016

Thank you for the work.
Will take a closer look at some point.
Agree that we can refactor the code and have less duplication

@@ -133,8 +136,7 @@ ShadowViewport.prototype.processCTM = function() {
this.originalState.x = newCTM.e
this.originalState.y = newCTM.f

// Update viewport CTM and cache zoom and pan
this.setCTM(newCTM);
Copy link
Contributor Author

@kevinlaw91 kevinlaw91 Sep 21, 2016

Choose a reason for hiding this comment

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

This line is only used by ShadowViewport.prototype.init(). So i moved it out and place it after calling processCTM() in init().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will allow reuse of processCTM() in SvgPanZoom.prototype.resize()

@kevinlaw91
Copy link
Contributor Author

@bumbu, I have refactored the code. You can test to see if it is ready for merge. This is a fix of behavior so could be a breaking change.

@bumbu
Copy link
Owner

bumbu commented Oct 3, 2016

Thanks @kevinlaw91
If it is a breaking change then it may be better to postpone it until next major release.
I'm really busy now and just can't find enough time for a bigger release, but I'll try as soon as possible.

Copy link
Contributor Author

@kevinlaw91 kevinlaw91 left a comment

Choose a reason for hiding this comment

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

Squashed everything and rebase to master

@kevinlaw91
Copy link
Contributor Author

Thanks for your time. FYI this is just a simple bug fix for an odd behavior. I believe the old behavior wasn't suppose to be a feature, so it may not be a breaking change either. Anyway, feel free to test it out when you have time. Hope to see this get merged before next major release. ✌️

@bumbu
Copy link
Owner

bumbu commented Nov 21, 2016

I tested it with different SVGs, also I paid attention to SVGs with viewBox attribute. All worked fine.
The changes make sense, and I agree that old behaviour shouldn't be a feature. Still some project may depend on that so I'll release it as a new minor version.

Thanks for the effort!

@bumbu bumbu merged commit 57fb063 into bumbu:master Nov 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants