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

Add max zoom limit #16

Merged
merged 5 commits into from
Feb 13, 2023
Merged

Add max zoom limit #16

merged 5 commits into from
Feb 13, 2023

Conversation

karolstawowski
Copy link
Contributor

  • Created a ZoomUtils static class with method to prevent from zooming out more than a 0.1 value with [-] button.
  • Added max zoom out control to zoomInOnMouse method of Coordinates class.

Additionally, I thought about setting a zoom to 0.1 when next zoom out action would set zoom to value smaller than 0.1.

Any feedback desirabled!

Resolves #13

@karolstawowski karolstawowski mentioned this pull request Feb 9, 2023
Copy link
Owner

@tom-mohr tom-mohr left a comment

Choose a reason for hiding this comment

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

Hey ;) thanks for your effort! The app is showing exactly the wanted behavior, that's cool! However, I have some change requests concerning the code:

  • Instead of a separate ZoomUtils class, just inline the code like case "-" -> zoomGoal = Math.max(0.1, zoomGoal / Math.pow(zoomStepFactor, 2)); (but with some MAX_ZOOM variable in Main.java)
  • Actually, it should probably be called "MIN_ZOOM", not "MAX_ZOOM", now that I think about it 😄
  • Setting a maximum for the zoom should not be something that the Coordinates class is concerned about. Maybe you can rewrite the method Coordinates.zoomInOnMouse so that it takes the new zoom value instead of the zoom increase? Then, we can simply pass Math.max(0.1, newZoom) as argument when calling the method from the Main class.

Hope you can work with my feedback, otherwise just let me know.

@karolstawowski
Copy link
Contributor Author

Hi! I'm happy the outcome of my work met your expectations in terms of functionality. I have made code changes according to your comment!

  • Moved [-] button logic to Main
  • Changed MAX_ZOOM to MIN_ZOOM (at the beginning I was curious why it was max zoom, but now we're fine 😄)
  • Added newZoom parameter to zoomInOnMouse method of Coordinates class. I could not figure out how to get rid of zoomFactor as a parameter - it is necessery to shift the camera by dividing original vector - maybe you have some idea on how to improve the code here?

Hope everything will be better this time! 😃

@tom-mohr tom-mohr mentioned this pull request Feb 13, 2023
@tom-mohr
Copy link
Owner

Hey, looking very good!👌I just removed the zoomFactor parameter from Coordinates.zoomInOnMouse(), you can see my change in #17 . If you don't have any further comments, I will merge #17 tomorrow. Thanks again! 😄

@karolstawowski
Copy link
Contributor Author

Okay, that makes sense now... Thank you for the opportunity to participate in the project! 😀

@tom-mohr tom-mohr merged commit f450dc7 into tom-mohr:main Feb 13, 2023
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.

Add max zoom limit
2 participants