fix: ensure draggable_scrollable_sheet respects shouldCloseOnMinExten… #156338

kirya355 · 2024-10-07T16:36:52Z

This PR fixes the shouldCloseOnMinExtent flag in draggable_scrollable_sheet.dart.

The shouldCloseOnMinExtent flag was not functioning correctly in the DraggableScrollableSheet widget. This PR ensures that the flag is properly handled, preventing the sheet from closing when it reaches the minimum extent if the flag is set to false. Before/after screenshots are not applicable for this change.

In the code sample below, before my fix, the sheet would close when scrolled down. After my fix, it behaves as expected by respecting the shouldCloseOnMinExtent parameter.

Code Sample

import 'package:flutter/material.dart';

Future<void> main() async {
  runApp(const MaterialApp(
    home: Home(),
  ));
}

class Home extends StatelessWidget {
  const Home({
    super.key,
  });

  @override
  Widget build(BuildContext context) => Scaffold(
        body: Center(
          child: FilledButton(
            onPressed: () async => showModalBottomSheet(
              context: context,
              isScrollControlled: true,
              isDismissible: false,
              builder: (context) => DraggableScrollableSheet(
                expand: false,
                maxChildSize: 0.9,
                minChildSize: 0.25,
                initialChildSize: 0.5,
                shouldCloseOnMinExtent: false,
                builder: (context, scrollController) => Navigator(
                  onGenerateRoute: (settings) => MaterialPageRoute(
                    settings: settings,
                    builder: (context) => ListView.builder(
                      controller: scrollController,
                      itemExtent: 100,
                      itemCount: 100,
                      itemBuilder: (context, index) => Center(
                        child: Text('$index'),
                      ),
                    ),
                  ),
                ),
              ),
            ),
            child: const Text('Open sheet'),
          ),
        ),
      );
}

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 this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

flutter-dashboard · 2024-10-07T16:36:55Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

nate-thegrate

Looks great!

Only thing we're missing is a test—gotta make sure we don't accidentally break or revert this in the future.

nate-thegrate · 2024-10-08T04:12:24Z

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

-    Duration? snapAnimationDuration,
-    bool shouldCloseOnMinExtent = true,
+    required Duration? snapAnimationDuration,
+    required bool shouldCloseOnMinExtent,


I guess whether these are required doesn't matter too much since it's a private class… I like how it looks though 👍

private class should have required since we own all the call site

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

chunhtai

change looks good to me, but we need a test

Piinks · 2024-10-22T22:14:07Z

(PR triage): @kirya355 thanks for contributing. Do you plan to continue working on this change?

kirya355 · 2024-10-24T08:14:25Z

(PR triage): @kirya355 thanks for contributing. Do you plan to continue working on this change?

Yes, I do plan to continue working on this change. Thank you for your attention!

kirya355 · 2024-10-24T10:37:14Z

I have added a test to verify that DraggableScrollableSheet respects the shouldCloseOnMinExtent property. Please take a look when you have a chance.
cc: @chunhtai, @nate-thegrate

nate-thegrate

Thanks @kirya355!

Would you be able to do a rebase so we can get those checks running?

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart

…t flag

…Extent

…dart Co-authored-by: Nate Wilson <[email protected]>

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart

nate-thegrate

Thanks for following up on this! Just have a couple of small suggestions :)

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart

…dart Co-authored-by: Nate Wilson <[email protected]>

nate-thegrate

Thanks for the contribution!

chunhtai

LGTM

chunhtai · 2024-11-05T22:26:17Z

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

-    Duration? snapAnimationDuration,
-    bool shouldCloseOnMinExtent = true,
+    required Duration? snapAnimationDuration,
+    required bool shouldCloseOnMinExtent,


private class should have required since we own all the call site

…MinExten… (flutter/flutter#156338)

flutter/flutter@29d40f7...73546b3 2024-11-06 [email protected] Add test for `image.loading_builder.0.dart` (flutter/flutter#158248) 2024-11-06 [email protected] Make flutter_tools use newest package:{native_assets_builder,native_assets_cli,native_toolchain_c} (flutter/flutter#158214) 2024-11-06 [email protected] Fix RawScrollbar examples and desktop test (flutter/flutter#158237) 2024-11-06 [email protected] Cleanup MenuAnchor and Improve DropdownMenu tests readability (flutter/flutter#158175) 2024-11-06 [email protected] Roll Flutter Engine from a3741d6248b7 to f03f11300a9d (2 revisions) (flutter/flutter#158222) 2024-11-06 [email protected] Update error message for Cocoapods support for synchronized groups/folders (flutter/flutter#158206) 2024-11-06 [email protected] Restore skipped iOS test by looping over `FakeAsync` elapse. (flutter/flutter#158204) 2024-11-06 [email protected] fix: ensure draggable_scrollable_sheet respects shouldCloseOnMinExtenâ�¦ (flutter/flutter#156338) 2024-11-06 [email protected] Roll Flutter Engine from e5e06c97c33c to a3741d6248b7 (14 revisions) (flutter/flutter#158218) 2024-11-06 [email protected] Forward fix `CupertinoDynamicColor` by adding `toARGB32()`. (flutter/flutter#158145) 2024-11-05 [email protected] Remove unused `enableObservatory` flag. (flutter/flutter#158202) 2024-11-05 [email protected] Remove observatory related TODO that is already fixed. (flutter/flutter#158205) 2024-11-05 [email protected] Factor out "shaker" class (flutter/flutter#157748) 2024-11-05 [email protected] Marks Mac_benchmark animated_complex_opacity_perf_macos__e2e_summary to be flaky (flutter/flutter#157424) 2024-11-05 [email protected] Increase subsharding for `Linux tool_integration_tests` (flutter/flutter#158196) 2024-11-05 [email protected] Add test for `raw_scrollbar.2.dart` (flutter/flutter#158161) 2024-11-05 [email protected] use root directory as the default for rootOverride in Cache.test constructor (flutter/flutter#158201) 2024-11-05 [email protected] Kill interactive script job `xcdevice observe` processes on tool/daemon shutdown (flutter/flutter#157646) 2024-11-05 [email protected] Fix: Gap between prefix and suffix icon and input field in input decoâ�¦ (flutter/flutter#152069) 2024-11-05 [email protected] Roll Flutter Engine from f56401062e42 to e5e06c97c33c (1 revision) (flutter/flutter#158194) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…MinExten… (flutter/flutter#156338)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Oct 7, 2024

nate-thegrate self-requested a review October 7, 2024 21:31

nate-thegrate reviewed Oct 8, 2024

View reviewed changes

chunhtai reviewed Oct 8, 2024

View reviewed changes

nate-thegrate self-requested a review October 24, 2024 19:37

nate-thegrate reviewed Oct 25, 2024

View reviewed changes

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart Outdated Show resolved Hide resolved

kirya355 and others added 3 commits October 25, 2024 12:01

fix: ensure draggable_scrollable_sheet respects shouldCloseOnMinExten…

3e745d5

…t flag

Add test to verify DraggableScrollableSheet respects shouldCloseOnMin…

70fb1ac

…Extent

Update packages/flutter/test/widgets/draggable_scrollable_sheet_test.…

38d50e0

…dart Co-authored-by: Nate Wilson <[email protected]>

kirya355 requested a review from nate-thegrate October 25, 2024 09:03

nate-thegrate reviewed Oct 25, 2024

View reviewed changes

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart Show resolved Hide resolved

kirya355 requested a review from nate-thegrate October 29, 2024 07:48

nate-thegrate reviewed Oct 30, 2024

View reviewed changes

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart Show resolved Hide resolved

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart Show resolved Hide resolved

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart Show resolved Hide resolved

kirya355 and others added 2 commits October 30, 2024 10:24

Update packages/flutter/test/widgets/draggable_scrollable_sheet_test.…

0af992d

…dart Co-authored-by: Nate Wilson <[email protected]>

Update packages/flutter/test/widgets/draggable_scrollable_sheet_test.…

a9d2f5d

…dart Co-authored-by: Nate Wilson <[email protected]>

kirya355 requested a review from nate-thegrate October 30, 2024 07:28

nate-thegrate approved these changes Oct 30, 2024

View reviewed changes

nate-thegrate requested a review from chunhtai October 30, 2024 21:10

chunhtai approved these changes Nov 5, 2024

View reviewed changes

Merge branch 'master' into master

0c8946a

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 5, 2024

auto-submit bot merged commit 7a57b69 into flutter:master Nov 6, 2024
68 checks passed

engine-flutter-autoroll mentioned this pull request Nov 6, 2024

Roll Flutter from 29d40f7f6826 to 73546b3b71a7 (20 revisions) flutter/packages#8028

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2024

7a57b69 fix: ensure draggable_scrollable_sheet respects shouldCloseOn…

661d388

…MinExten… (flutter/flutter#156338)

Piinks mentioned this pull request Nov 8, 2024

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

7a57b69 fix: ensure draggable_scrollable_sheet respects shouldCloseOn…

8fc6506

…MinExten… (flutter/flutter#156338)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

7a57b69 fix: ensure draggable_scrollable_sheet respects shouldCloseOn…

9db6df0

…MinExten… (flutter/flutter#156338)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

7a57b69 fix: ensure draggable_scrollable_sheet respects shouldCloseOn…

29207be

…MinExten… (flutter/flutter#156338)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

7a57b69 fix: ensure draggable_scrollable_sheet respects shouldCloseOn…

b5fc7a9

…MinExten… (flutter/flutter#156338)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

7a57b69 fix: ensure draggable_scrollable_sheet respects shouldCloseOn…

788145b

…MinExten… (flutter/flutter#156338)

fix: ensure draggable_scrollable_sheet respects shouldCloseOnMinExten… #156338

fix: ensure draggable_scrollable_sheet respects shouldCloseOnMinExten… #156338

Uh oh!

Conversation

kirya355 commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Sample

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 7, 2024

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

chunhtai Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Oct 22, 2024

Uh oh!

kirya355 commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kirya355 commented Oct 24, 2024

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kirya355 commented Oct 7, 2024 •

edited

Loading

kirya355 commented Oct 24, 2024 •

edited

Loading