-
Notifications
You must be signed in to change notification settings - Fork 54
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
margins: now in user units, and working for all aspect ratios cases #86
Conversation
Not sure how you test this, I used https://codepen.io/jonenst/pen/mdrgwoJ and copypaste to a local file and imported the build like The code can probably become shorter by using more symmetries between meet and slice, but I wasn't sure it was worth it, I understand this one better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you doublecheck that?
src/svg.panzoom.js
Outdated
preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMIN || | ||
preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMIN || | ||
preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMIN) { | ||
viewportLeftOffset = -widthOffset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looking at all the symmetry going on, this should be positive...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this should be viewportRightOffset
and the other one viewportLeftOffset
src/svg.panzoom.js
Outdated
preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMINYMAX || | ||
preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX || | ||
preserveAspectRatio.align === preserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMAXYMAX) { | ||
viewportRightOffset = widthOffset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...while this should be negative.
oh no I hope the whole symmetry is not different now :D. I shorted the code by a lot using symmetry. we'll see ^^ |
Yeah the slice mode was broken, the testing XMid/Min/Max and YMid/MinMax were swapped, and there was the bug you noticed, assigning to the wrong variable |
see #80