Add Snapping Behavior to DraggableScrollableSheet by caseycrogers · Pull Request #84394 · flutter/flutter

caseycrogers · 2021-06-10T23:11:42Z

This PR adds two new optional arguments to DraggableScrollableSheet called snap and snapSizes. If snap is false (default value), the widget's behavior remains unchanged. If snap is true, the widget will snap to snapSizes when the user's finger leaves the screen during a drag. See for details: #34111 (comment).

This is achieved by replacing (if a snap should occur) the sheet's ballistic simulation in _DraggableScrollableSheetExtent.goBallistic with one that snaps to targets instead of moving forward until velocity is dissipated.

Fixes: #34111

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2021-06-10T23:11:44Z

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 on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

google-cla · 2021-06-10T23:11:52Z

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 @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

caseycrogers

Open questions/comments that I'd love to get feedback on from someone with more Flutter experience than me!

caseycrogers · 2021-06-11T16:21:41Z

@googlebot I signed it!

caseycrogers · 2021-06-11T21:26:49Z

@dnfield Hello Dan!

This PR closes out #34111. I noticed you've previously contributed to DraggableScrollableSheet and you were active in the above issue. This PR is still WIP (namely, behavior appears to work but there aren't any unit tests). Before I write my tests, I was wondering if you could give me a pre-review on the PR to let me know if I'm taking the right approach? Or direct me to the right reviewer?

dnfield

/cc @goderbauer

Alexays · 2021-06-19T22:21:38Z

Just a comment about the behavior @caseycrogers, using isScrollingNotifier.value makes the UX weird, When the user removes the finger from the screen, the sheet should extend directly to the nearest snap point, instead of waiting until there is no more velocity (idle) and then extends.

caseycrogers · 2021-06-29T15:21:16Z

Just a comment about the behavior @caseycrogers, using isScrollingNotifier.value makes the UX weird, When the user removes the finger from the screen, the sheet should extend directly to the nearest snap point, instead of waiting until there is no more velocity (idle) and then extends.

Yeah this is a good point. I've been pretty distracted lately but will get back to this in a bit. When I do I'll look for a way to snap immediately when the user's finger is released.

caseycrogers · 2021-08-16T20:13:28Z

Posted in the issue but is https://pub.dev/packages/snapping_sheet a better solution in this case?

Responded in the issue!

caseycrogers · 2021-08-18T17:53:09Z

@dnfield let me know if you want any more input from me before reviewing!

dnfield

Looking closer!

skia-gold · 2021-08-19T23:01:46Z

Gold has detected about 1 new digest(s) on patchset 54.
View them at https://flutter-gold.skia.org/cl/github/84394

caseycrogers

@dnfield Resolved formatting comments. I had two questions (see open conversations), once I get more feedback I'll push a commit and resolve them.

b3nni97 · 2021-09-08T23:46:06Z

Hi I have tested your PR and would prefer to have a control how the snapping animation runs. (setting curves etc.)

caseycrogers · 2021-09-09T00:52:47Z

@@ -298,12 +353,38 @@ class _DraggableScrollableSheetState extends State<DraggableScrollableSheet> {
    _extent = _DraggableSheetExtent(


@dnfield It's just occurred to me that this code in prod and in my branch is a bit broken:
DraggableScrollableSheet does not update the extent when the widget is rebuilt with new values. This means that the extent will retain potentially invalid values for min, max, snapSizes, etc. if the user rebuilds the sheet with new parameters.

We need to override DidUpdateWidget to rebuild the extent when necessary. This feels fairly non-trivial and is largely about fixing existing behavior in prod-are you okay with me leaving this fix to a follow up PR to go in after this one merges? Or should I spin up a new PR now to fix it and rebase this one on top of it? Seems like a fairly severe bug that may justify the latter.

Here's a (failing in prod) test that can be added to draggable_scrollable_sheet_test.dart to demonstrate the issue:

testWidgets('A changed maxChildSize is propagated', (WidgetTester tester) async { const Key stackKey = ValueKey<String>('stack'); const Key containerKey = ValueKey<String>('container'); await tester.pumpWidget(_boilerplate(null, snap: true, stackKey: stackKey, containerKey: containerKey, snapSizes: <double>[.5], )); await tester.pumpAndSettle(); final double screenHeight = tester.getSize(find.byKey(stackKey)).height; await tester.drag(find.text('Item 1'), Offset(0, -.4 * screenHeight)); await tester.pumpAndSettle(); expect( tester.getSize(find.byKey(containerKey)).height / screenHeight, closeTo(1.0, precisionErrorTolerance), ); // Pump the same widget but with a new snapSizes, triggering a rebuild. await tester.pumpWidget(_boilerplate(null, snap: true, stackKey: stackKey, containerKey: containerKey, maxChildSize: .8, snapSizes: <double>[.5], )); await tester.pumpAndSettle(); // The max child size has been reduced, we should be rebuilt at the new // max of .8. expect( tester.getSize(find.byKey(containerKey)).height / screenHeight, closeTo(.8, precisionErrorTolerance), ); }, variant: TargetPlatformVariant.all());

Doing this in a new PR is fine. https://github.com/flutter/flutter/pull/69724/files probbly would fix this, and relanding that without the bit about caching the child is probably a good idea.

dnfield · 2021-09-09T18:34:42Z

+    for (int index = 0; index < (widget.snapSizes?.length ?? 0); index += 1) {
+      final double snapSize = widget.snapSizes![index];
+      assert(snapSize >= widget.minChildSize && snapSize <= widget.maxChildSize,
+        'Snap sizes must be between `minChildSize` and `maxChildSize`. Invalid size: $snapSize');


nit: here and below, probably good to dump the whole list and point to the invalid one rather than just printing the invalid one, which might occur multiple times in a badly formed list.

Fix'd!

Invalid snapSize '0.4' at index 3 of: [0.3, 0.4, 0.5, >>> 0.4 <<<, 0.6, 0.7] Snap sizes must be in ascending order.

However, I'm getting the canned Flutter message "Either the assertion indicates an error in the framework itself, or we should..." message. It looks like it's just naively checking if the top two frames of the stack are Flutter and not user code? Do you know any ways to get rid of this? I guess putting the assert in the constructor would solve this but then (as discussed earlier) we lose const and that's a breaking change.

Maybe losing const is worth moving the asserts up? Potentially a breaking-change follow up PR?

Feel free to file a bug for that - that code has a history of breaking :\

File a bug for moving the asserts into the constructor, or file a bug for the "Either the assertion..." message firing?

For the Either the assertion message :)

dnfield

LGTM with nitpick.

Thanks for sticking with this.

If you plan to support supplying custom curves for the animation, maybe we can do that in a separate PR?

caseycrogers · 2021-09-09T19:02:37Z

LGTM with nitpick.

Thanks for sticking with this.

If you plan to support supplying custom curves for the animation, maybe we can do that in a separate PR?

NP, thanks for all the feedback!

Happy to do that in a separate PR. I fixed the nit, but there's still one minor issue with the assert message (responded in comment thread).

caseycrogers · 2021-09-15T17:39:57Z

@dnfield How do we get this merged? It looks like the tests are permanently pending?

dnfield · 2021-09-15T17:42:47Z

Hm. Can you push up an empty commit to kick CI? Something seems to have gotten hung up.

caseycrogers · 2021-09-15T18:19:09Z

Things appear very busted :/. I tried git fetch upstream and git merge master, neither fetched any new commits. should I post in hackers discord?

Edit: looks like my upstream was configured improperly so git fetch upstream was doing nothing. Should be resolved now.

dnfield · 2021-09-15T20:04:53Z

Looks like ci is running now

Piinks · 2021-09-15T20:22:52Z

It looks like the git issue here may have broken the flutter-gold check. I will watch this and merge manually once the Google testing shard passes, hoping this will unblock the flutter-gold check.

Piinks · 2021-09-15T20:30:26Z

The google check also appears broken here, I am going to merge this manually. I confirmed the google testing shard passed.

MalteMagnussen · 2021-09-16T19:06:39Z

Thanks a lot for this!
Great job @caseycrogers

om-ha · 2021-10-11T20:58:38Z

This PR is so important to our project and I can't wait for a stable release. Because of so, I took the file draggable_scrollable_sheet.dart from this PR, and applied it to my project.

This is how I did that:

Replace relative imports within this file with a simple

import 'package:flutter/widgets.dart';

At call-site, I had to do this:

Option 2.A:

import 'package:flutter/material.dart' hide DraggableScrollableSheet;
import 'package:your_project/src/third_party/draggable_scrollable_sheet_PR_84394/draggable_scrollable_sheet.dart';

Option 2.B:

import 'package:flutter/material.dart';
import 'package:your_project/src/third_party/draggable_scrollable_sheet_PR_84394/draggable_scrollable_sheet.dart' as draggable_scrollable_sheet_mod;

This is a bit hacky but manageable with proper README explaining what's happening. Sometimes I would rather use first-party packages than third-party. No regrets!

Kudos to the PR author, Googlers, and community that helped push this.

caseycrogers · 2021-10-12T22:35:55Z

@om-ha Also note (and for others reading this) that the snapping functionality is already out on the beta channel if you'd rather use it that way!

See switching channels.

caseycrogers added 3 commits June 10, 2021 15:44

hijack helloworld

043c779

added snap points and snaping behavior

a198f8a

working in basic scenario

2faae03

flutter-dashboard Bot added d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jun 10, 2021

google-cla Bot added the cla: no label Jun 10, 2021

fixed trivial merg conflict

6925ec6

google-cla Bot added cla: yes and removed cla: no labels Jun 10, 2021

caseycrogers mentioned this pull request Jun 10, 2021

Expandable Bottom Sheet ueman/feedback#89

Closed

5 tasks

caseycrogers marked this pull request as draft June 10, 2021 23:20

caseycrogers commented Jun 10, 2021

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart Outdated

caseycrogers added 2 commits June 10, 2021 16:54

hand resolved formatting

805fbba

nuked accidental files

afedcf4

caseycrogers added 5 commits June 11, 2021 09:24

re-added files I deleted

b0bbd5e

undid formatting changes

5d5eb3b

revert to list

03af82a

First fully working implementation!

f0d18cb

updated naming

4f7c233

caseycrogers commented Jun 11, 2021

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart Outdated

s/snapPoints/snapTargets

28aaf23

dnfield reviewed Jun 12, 2021

View reviewed changes

fixed accidental set

a0ec3bc

Piinks added f: scrolling Viewports, list views, slivers, etc. c: new feature Nothing broken; request for a new capability labels Aug 17, 2021

dnfield reviewed Aug 18, 2021

View reviewed changes

Minor formatting, switched to canonical epsilon

02e4048

caseycrogers commented Aug 19, 2021

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

Comment thread packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

goderbauer requested a review from dnfield September 8, 2021 21:53

caseycrogers commented Sep 9, 2021

View reviewed changes

dnfield reviewed Sep 9, 2021

View reviewed changes

dnfield approved these changes Sep 9, 2021

View reviewed changes

improved snap sizes error messsage

fcf1ff9

caseycrogers mentioned this pull request Sep 10, 2021

DraggableScrollableSheet with dynamic height #41599

Closed

triggering CI rebuild

4e7ed11

Merge branch 'master' into snapping_sheet

ee6cd07

dnfield added the waiting for tree to go green label Sep 15, 2021

Piinks merged commit e7df5ec into flutter:master Sep 15, 2021

caseycrogers deleted the snapping_sheet branch September 15, 2021 21:35

		@@ -298,12 +353,38 @@ class _DraggableScrollableSheetState extends State<DraggableScrollableSheet> {
		_extent = _DraggableSheetExtent(

Conversation

caseycrogers commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard Bot commented Jun 10, 2021

Uh oh!

google-cla Bot commented Jun 10, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

caseycrogers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

caseycrogers commented Jun 11, 2021

Uh oh!

Uh oh!

caseycrogers commented Jun 11, 2021

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alexays commented Jun 19, 2021

Uh oh!

caseycrogers commented Jun 29, 2021

Uh oh!

caseycrogers commented Aug 16, 2021

Uh oh!

caseycrogers commented Aug 18, 2021

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skia-gold commented Aug 19, 2021

Uh oh!

caseycrogers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

b3nni97 commented Sep 8, 2021

Uh oh!

caseycrogers Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

caseycrogers Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

caseycrogers Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

caseycrogers Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 10, 2021

Choose a reason for hiding this comment

caseycrogers commented Jun 10, 2021 •

edited

Loading

caseycrogers Sep 9, 2021 •

edited

Loading

caseycrogers Sep 9, 2021 •

edited

Loading

caseycrogers commented Sep 9, 2021 •

edited

Loading

caseycrogers commented Sep 15, 2021 •

edited

Loading

om-ha commented Oct 11, 2021 •

edited

Loading

caseycrogers commented Oct 12, 2021 •

edited

Loading