-
Notifications
You must be signed in to change notification settings - Fork 28.7k
Fix draggable scrollable sheet scroll notification #45083
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 draggable scrollable sheet scroll notification #45083
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Hi @itome, thanks for the PR! Could you please add a test that fails on master, and succeeds with your PR added? That way we can prevent a regression of your change in the future. |
c3a3dc3
to
768732e
Compare
@gspencergoog I added tests! |
|
||
final List<Type> types = <Type>[ | ||
ScrollStartNotification, | ||
ScrollEndNotification, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gspencergoog Do you think UserScrollNotification
and ScrollUpdateNotification
should also be dispatched while dragging without covering its container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the docs on https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/scroll_notification.dart#L43, it seems like there should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, it seems like in this case we could file a seaprate bug for that and fix it in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(With a TODO here for it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added TODO comment. And I will send another PR for this bug. Do I need to post issue before sending PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issuebis helpful to make sure we don't lose track
expect(notificationTypes.removeAt(0), ScrollStartNotification); | ||
expect(notificationTypes.removeAt(0), UserScrollNotification); | ||
expect(notificationTypes.removeLast(), UserScrollNotification); | ||
expect(notificationTypes.removeLast(), ScrollEndNotification); | ||
expect( | ||
notificationTypes.every((Type t) => t == ScrollUpdateNotification), | ||
true, | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many notifications are we getting from this? Can we just assert we get the exact expected number and order of notifications instead of this logic? Maybe something like:
final List<Type> expectedNotificationTypes = [
ScrollStartNotification,
UserScrollNotification,
...List<Type>.filled(expectedNumberOfUpdates, ScrollUpdateNotifcation),
UserScrollNotification,
ScrollEndNotification,
];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I fixed test code.
c1abbcf
to
2dce8d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
||
final List<Type> types = <Type>[ | ||
ScrollStartNotification, | ||
ScrollEndNotification, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issuebis helpful to make sure we don't lose track
@dnfield CI failing bug it seems to be not related to my changes. Can you rerun it? |
Restarted. |
So, I did restart it, but it isn't flake. Can you try syncing to HEAD and pushing again? You might just be synced to a bad build. |
Thanks, CI passed. |
I need to dismiss the draggable bottom sheet when clicking on the background of the bottom sheet model. |
Description
ScrollEndNotification for DraggableScrollable's child ScrollView doesn't dispatched when inertia scroll ended in its extent. And then, it is dispatched when next drag started.
To fix this, I added
super.goBallastic(0)
in_DraggableScrollableSheetScrollPosition.goBallastic()
when inertia scroll ended.Related Issues
Replace this paragraph with a list of issues related to this PR from our [issue database]. Indicate, which of these issues are resolved or fixed by this PR.
Tests
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?
ScrollEndNotification is dispatched at the right time