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

Fix bounds Issue (for #1362) #1369

Merged
merged 1 commit into from
Sep 23, 2022
Merged

Conversation

ibrierley
Copy link
Collaborator

Possible fix for the recent bounds issue. Needs proper testing before merging as I'm not quite familiar with bounds and recent changes.

What this does is remove the fitBounds in the build method (I'm not sure this is needed there at all, as long as its run once?), and move it just to the initState (but it looks like it needs to run in the WidgetsBinding.instance.addPostFrameCallback for it to take effect...I'm not entirely sure if this is best or if its better before/after options.onMapReady?.call()).

Few thoughts...should bounds (I think we should think of it as initialFitBounds if my understanding of it is correct, and maybe rename one day) only run here...are there any other situations when we need to reset bounds ? Are there any possible race conditions where this is in the wrong place.

We can test the basics with something like this in one of the examples...

options: MapOptions(
                    bounds: LatLngBounds(
                      LatLng(56.6877, 11.5089),
                      LatLng(56.7378, 11.6644),
                    )
                ),

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Sep 22, 2022

I will try to have a look into this tomorrow when I get a moment, as this is high priority. Just to note, I'm not totally happy with bounds and things either, so maybe @moonag or @TesteurManiak want to have a look.
(EDITED)

@JaffaKetchup JaffaKetchup changed the title possible fix for bounds breaking and limiting drag Fix bounds Issue (for #1362) Sep 22, 2022
@JaffaKetchup JaffaKetchup linked an issue Sep 22, 2022 that may be closed by this pull request
5 tasks
@JaffaKetchup JaffaKetchup added bug This issue reports broken functionality or another error high priority labels Sep 22, 2022
@mootw
Copy link
Collaborator

mootw commented Sep 22, 2022

This is probably fine. The logic of fitbounds can probably be refactored to get the state to apply before the first build. Not sure why it wouldn't apply from setState though. I would have to look further. I don't have the time to right now, but this change probably won't severely break things. I have not tested this change, just looked at the code.

@ibrierley
Copy link
Collaborator Author

Would prefer others to test really on this one, as there may be some edge cases I haven't thought of, especially with initialisation and timing.
Would also be helpful if those who had the problem could give it a test to check it fixes it for them.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM. It's resolved the issue, and I cannot see why it would have side effects.

@JaffaKetchup JaffaKetchup merged commit 678a8ab into fleaflet:master Sep 23, 2022
@ibrierley
Copy link
Collaborator Author

Great, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue reports broken functionality or another error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Can't do any interaction on the map if I pass bounds param
3 participants