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

Force move to pick center even if max bounds does not fit #1331

Closed
wants to merge 3 commits into from

Conversation

jetpeter
Copy link
Contributor

@jetpeter jetpeter commented Aug 2, 2022

If bounds do not fit first attempt to zoom up to the max zoom.
If center still cannot work then just set center with max zoom
and live with the grey borders.

@ibrierley
Copy link
Contributor

This feels like it "jumps" too much, rather than sliding into place like it does currently (though this would fix it on an init better). Quick video https://drive.google.com/file/d/1m5oDdjuuvtE10PIKd3EjBRWp-jzWe3Hv/view to try and explain it a bit!

@jetpeter
Copy link
Contributor Author

jetpeter commented Aug 2, 2022

Yes I agree, It works good with a really small maxBounds but breaks down with larger areas. Im working on making an adjustment.

@ibrierley
Copy link
Contributor

Does the original work for you apart from the init phase ? Just wondering if that could be an option, rather than changing behaviour for people that exists already.

@ibrierley
Copy link
Contributor

(hmm I guess you never get past that to know!)

@jetpeter
Copy link
Contributor Author

jetpeter commented Aug 2, 2022

It does work actually. The problem is init() calls move or setBounds which also eventually calls move. I was avoiding adding a new boolean param to the move method. Im experimenting now with only zooming if _lastCenter is null.

@jetpeter
Copy link
Contributor Author

jetpeter commented Aug 2, 2022

Has anyone else ran into an issue where you cannot pan the map at some locations when zoomed out fully and and rotated. I assumed it was a bug I created, but its showing up in the main branch as well. If you zoom and and then zoom out again you can pan but if you just rotate from all the way zoomed out it does not work. I'll try to address that too.

@jetpeter
Copy link
Contributor Author

jetpeter commented Aug 2, 2022

I think the map being frozen is an expected side effect of being zoomed all the way out with maxBounds fixing it would require adjusting zoom when panning which would be unpleasant.

@JaffaKetchup
Copy link
Member

Sorry @jetpeter, bit out of the loop, is there anything you need me to do?

@jetpeter
Copy link
Contributor Author

@JaffaKetchup This just needs re-review. @ibrierley made a good point about the map jumping when you hit bound which was caused by the re-center zoom adjustment being called when it didn't need to be. I have since changed the code to only adjust zoom for max bounds if the late non null variables that are set in the move method are null. This means that the code should only be run during the first call to move which is from the init method. The change now should have not negative impact on existing users. The only change users will see is the map is zoomed more on devices where the view port size is smaller than the maxBounds, or in the worst case they will see their max bounds in the center of the map the most zoomed in it can be. There should never be an exception when using maxBounds when using this change.

If bounds do not fit first attempt to zoom up to the max zoom.
If center still cannot work then just set center with max zoom
and live with the grey borders.
@JaffaKetchup
Copy link
Member

@jetpeter I'm giving priority to #1333 for now, as it's a breaking change. Then I'll review and merge also as part of v3.0.0. Thanks for your contributions so far, and sorry for the delay :)

Copy link
Contributor

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

I think I'm ok with this. It seems to test ok in the example, which was its original intent. I'm not necessarily a fan of changing a zoom that isn't solicited (wondering if there's some odd edge cases then), but neither am I fan of getting exceptions :D.

@JaffaKetchup
Copy link
Member

Hi @jetpeter, I'm struggling to resolve the conflicts after #1333. If possible, could you resolve the conflicts please? I've had a go myself, but the _lastCenter variable seems to have vanished into thin air.

@jetpeter
Copy link
Contributor Author

Yea, I'll get it done today

@JaffaKetchup
Copy link
Member

Closing as discussed in Discord server.

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.

3 participants