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

boundary constraint logic fix #2246

Merged
merged 3 commits into from Nov 30, 2022

Conversation

pearcetm
Copy link
Contributor

Fixes the behavior described and shown in a video here: #2239 (comment)

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

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

@pearcetm Thank you for the additional description and the video. This is definitely an improvement! I feel like we wrestled with this code for a good while in #2160; it's wild you could make it better by just deleting some code. 😆

I've played around with this a bit and it seems solid. @joedf Do you have any thoughts on this patch? Anything we should be testing before merging it?

src/viewport.js Outdated Show resolved Hide resolved
@joedf
Copy link
Contributor

joedf commented Nov 30, 2022

Works flawlessly, I'd say even much better! I recommend merging it and including this as is.
Only a minor thing is adding an extra empty line above line 536 for consistent formatting.

@pearcetm @iangilman Thanks for getting this fixed and verified! This whole constraints thing has definitely been a bit of a headache for me. I'm so glad, I knew it could be shorter, but just too confused. hahah 😬👍

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you for trying it out @joedf, and of course, thank you for making it happen, @pearcetm! Let's merge this! :)

@iangilman iangilman merged commit c92fe11 into openseadragon:master Nov 30, 2022
iangilman added a commit that referenced this pull request Nov 30, 2022
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.

None yet

3 participants