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

bugfix: Prevent parent ListView widgets from snatching scrolling gestures #994

Merged
merged 6 commits into from
Sep 3, 2021

Conversation

Binabh
Copy link
Contributor

@Binabh Binabh commented Aug 11, 2021

This is basically copy of #717 (since the PR creator does not seem intrested in implementing the commented option and PR is now stale). I saw that the PR is not being merged because there is no option to developer if they want previous behavior. I have tried to add a bool in mapoptions to determine the type of behavior in the map.

Let me know if anything else is required. I want this feature to be implemented.
Thank You!

@rknell
Copy link

rknell commented Aug 12, 2021

I've just tested this out and it works as advertised without any noticeable issues.

However the name of the flag wasn't very intuitive and its implemented as being on by default so it would be a breaking change.

I would suggest something like "captureGesturesInScrollview: true" and have it off by default. But im not a package maintainer so just my 2c.

Otherwise great work, I was just about to go about implementing it myself for a project due out tomorrow.

@Binabh
Copy link
Contributor Author

Binabh commented Aug 12, 2021

However the name of the flag wasn't very intuitive and its implemented as being on by default so it would be a breaking change.

I would suggest something like "captureGesturesInScrollview: true" and have it off by default. But im not a package maintainer so just my 2c.

Thank You for your comment. I think It is not a big issue to change that. I can do it. I will wait for review from maintainers and make all the changes along with this if they also suggest any.

@rknell
Copy link

rknell commented Aug 12, 2021

Yeah absolutely! Their opinion matters, I really just wanted to let them know its working for me in the wild which gives them some confidence to merge the pr in any form.

@kengu
Copy link
Contributor

kengu commented Sep 2, 2021

@Binabh Can you apply formatting and push the changes? Just run dart format . from repo-root.

@Binabh
Copy link
Contributor Author

Binabh commented Sep 3, 2021

@kengu done. let me know if anything else is needs to be modified

Copy link
Contributor

@kengu kengu left a comment

Choose a reason for hiding this comment

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

LGTM. See my comment on code-duplication. When this is fixed, I'll approve and merge.

lib/src/map/flutter_map_state.dart Outdated Show resolved Hide resolved
@Binabh
Copy link
Contributor Author

Binabh commented Sep 3, 2021

@kengu fixed the code duplication issue. let me know if anything else required

@Binabh Binabh requested a review from kengu September 3, 2021 09:44
Copy link
Contributor

@kengu kengu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

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