Add enableDrag property to CupertinoSheetRoute and showCupertinoSheet #163923
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Thank you for submitting this PR! Test looks great. Ideally I'd like to keep the buildPageTransitions method static. It might break some people to change it now, and it seems like we should be able to keep it static.
Besides that, I have some small comments on the documentation.
| /// Returns a [CupertinoSheetTransition]. | ||
| static Widget buildPageTransitions<T>( | ||
| Widget buildPageTransitions( |
There was a problem hiding this comment.
We can keep this as static if we add an enableDrag argument, then pass it in inside of buildTransitions. Keeping it static is more likely to help with performance.
There was a problem hiding this comment.
fix this commit
9720184
| @@ -581,8 +584,13 @@ mixin _CupertinoSheetRouteTransitionMixin<T> on PageRoute<T> { | ||
| ); | ||
| } | ||
|
|
||
| /// Determines whether the content can be scrolled. |
There was a problem hiding this comment.
Nit: replace "scroll" with "drag". So "can be dragged to dismiss the sheet", "dragging is enabled". Scrolling refers to a slightly different interaction.
There was a problem hiding this comment.
Also, it would be better if this was moved above the buildPage method https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#order-other-class-members-in-a-way-that-makes-sense
There was a problem hiding this comment.
fix this commit
0ebeb39
| @@ -1076,5 +1076,74 @@ void main() { | ||
| await gesture.up(); | ||
| await tester.pumpAndSettle(); | ||
| }); | ||
|
|
||
| testWidgets('dragging does not move the sheet when enableDrag is false', ( |
|
@MitchellGoodwin |
|
I'm not sure how much additional work it would take to add support for Sheet Detents, which adds to the size support to the sheets (https://developer.apple.com/design/human-interface-guidelines/sheets), in this same PR. |
|
@KlausJokisuo Therefore, I think PR should be separated. |
|
In general it's much better if work can be separated into different PRs. So even if it was simple to detents, it would be preferable if that was done in a separate PR. It makes things like documentation and diagnosing tree errors much cleaner. |
… the sheet can be dragged.(flutter#163852)
d3031ef to
7a0035c
Compare
|
Rebased it since Google testing failure was infra related. |
|
autosubmit label was removed for flutter/flutter/163923, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…flutter#163923) Implemented. (flutter#163852) This PR adds the `enableDrag` option to showCupertinoSheet and CupertinoSheetRoute, allowing developers to control whether the sheet can be dismissed by dragging. ## How to Verify - Set enableDrag: false in CupertinoSheetRoute or showCupertinoSheet. ```dart CupertinoSheetRoute<void>( builder: (BuildContext context) => const _SheetScaffold(), enableDrag: false, ), // or showCupertinoSheet<void>( context: context, useNestedNavigation: true, pageBuilder: (BuildContext context) => const _SheetScaffold(), enableDrag: false, ); ``` - The following is a screenshot of `examples/api/lib/cupertino/sheet/cupertino_sheet.0.dart` after adding `enableDrag: false`. https://github.com/user-attachments/assets/315fb0e5-ceee-4150-be6e-dd919a7c2317 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] 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. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
Implemented. (#163852)
This PR adds the
enableDragoption to showCupertinoSheet and CupertinoSheetRoute, allowing developers to control whether the sheet can be dismissed by dragging.How to Verify
examples/api/lib/cupertino/sheet/cupertino_sheet.0.dartafter addingenableDrag: false.2025-02-22.23.10.04.mov
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.