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.

Sign up for GitHub

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

Jump to bottom

Add enableDrag property to CupertinoSheetRoute and showCupertinoSheet #163923

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add enableDrag property to CupertinoSheetRoute and showCupertinoSheet #163923

wants to merge 5 commits into from

Conversation

masal9pse
Copy link

@masal9pse masal9pse commented Feb 22, 2025 •
edited
Loading

Implemented. (#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.
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.
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.

@google-cla Google CLA
Copy link

google-cla bot commented Feb 22, 2025

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Feb 22, 2025
@masal9pse masal9pse changed the base branch from main to master February 22, 2025 11:01
@masal9pse masal9pse changed the title Add the enableDrag property to CupertinoSheetRoute to control whether… Add enableDrag property to CupertinoSheetRoute and showCupertinoSheet Feb 22, 2025
@masal9pse masal9pse marked this pull request as ready for review February 22, 2025 15:02
MitchellGoodwin
MitchellGoodwin reviewed Mar 13, 2025
View reviewed changes
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

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.

packages/flutter/lib/src/cupertino/sheet.dart Outdated
/// Returns a [CupertinoSheetTransition].
static Widget buildPageTransitions<T>(
Widget buildPageTransitions(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

fix this commit
9720184

packages/flutter/lib/src/cupertino/sheet.dart Outdated
@@ -581,8 +584,13 @@ mixin _CupertinoSheetRouteTransitionMixin<T> on PageRoute<T> {
);
}

/// Determines whether the content can be scrolled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: replace "scroll" with "drag". So "can be dragged to dismiss the sheet", "dragging is enabled". Scrolling refers to a slightly different interaction.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

fix this commit
0ebeb39

packages/flutter/test/cupertino/sheet_test.dart
@@ -1076,5 +1076,74 @@ void main() {
await gesture.up();
await tester.pumpAndSettle();
});

testWidgets('dragging does not move the sheet when enableDrag is false', (
Copy link
Contributor

Choose a reason for hiding this comment

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

Great 👍

@masal9pse
Copy link
Author

@MitchellGoodwin
Thank you for the review! I've addressed the feedback, so please take a look when you have a moment.

@KlausJokisuo
Copy link

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.

@masal9pse 🙏

@masal9pse
Copy link
Author

masal9pse commented Mar 15, 2025 •
edited
Loading

@KlausJokisuo
After lightly modifying the dragEnd method and then changing to Medium detent size, there are some things that need to be fixed, such as the back screen not being fully displayed, and I don't think this feature can be released with a simple fix.........

Therefore, I think PR should be separated.

@MitchellGoodwin
Copy link
Contributor

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.

MitchellGoodwin
MitchellGoodwin approved these changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the PR! Next this needs a second reviewer. I'll ask around.

chunhtai
chunhtai approved these changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@MitchellGoodwin MitchellGoodwin force-pushed the add-enabledrag-cupertinosheet branch from d3031ef to 7a0035c Compare March 18, 2025 21:48
@MitchellGoodwin
Copy link
Contributor

Rebased it since Google testing failure was infra related.

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 19, 2025
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Mar 19, 2025

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.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chunhtai chunhtai chunhtai approved these changes

@MitchellGoodwin MitchellGoodwin MitchellGoodwin approved these changes

Assignees
No one assigned
Labels
f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@masal9pse @KlausJokisuo @MitchellGoodwin @chunhtai @dkwingsmt