Repository navigation
Add drag handle to CupertinoSheet - #179962
Conversation
github-actions
Bot
added
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a drag handle to CupertinoSheet. The implementation looks good overall. I've added a couple of comments to improve the code. One is a minor documentation fix. The other is a more significant refactoring of the _dragHandleBuilder method to improve its robustness and maintainability by replacing magic numbers with constants, using Padding instead of a MediaQuery hack, and removing commented-out code.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/flutter/lib/src/cupertino/sheet.dart (582-619)
This method has a few areas for improvement:
- Commented-out code: There's a block of commented-out code that should be removed.
- Magic numbers: The dimensions and padding for the drag handle are hardcoded. It's better to define them as named constants for clarity and maintainability.
- Content padding: Using
MediaQuerywithviewInsetsto push down the content is fragile, as not all widgets respectviewInsets. APaddingwidget is a more robust way to create space for the drag handle. - Incorrect
const: Theconstkeyword onAlignis incorrect because its child widget tree contains non-constant values.
Here's a suggested refactoring that addresses these points. You could define the constants at the top of the file or as static members of the class.
Widget _dragHandleBuilder(BuildContext context) {
if (!showDragHandle) {
return builder(context);
}
const double dragHandleSheetVerticalInset = 30.0;
const double dragHandleTopPadding = 5.0;
const double dragHandleHeight = 5.0;
const double dragHandleWidth = 36.0;
const double dragHandleBorderRadius = 18.0;
return Stack(
children: <Widget>[
Padding(
padding: const EdgeInsets.only(top: dragHandleSheetVerticalInset),
child: builder(context),
),
Align(
alignment: Alignment.topCenter,
child: Semantics(
properties: const SemanticsProperties(role: SemanticsRole.dragHandle),
child: Padding(
padding: const EdgeInsets.only(top: dragHandleTopPadding),
child: DecoratedBox(
decoration: ShapeDecoration(
shape: RoundedSuperellipseBorder(
borderRadius: const BorderRadius.all(Radius.circular(dragHandleBorderRadius)),
),
color: CupertinoColors.tertiaryLabel,
),
child: const SizedBox(height: dragHandleHeight, width: dragHandleWidth),
),
),
),
),
],
);
}packages/flutter/lib/src/cupertino/sheet.dart (124-126)
There's a small typo ("a an") and the property name is incorrect. It should be showDragHandle. Please also format property names as code using backticks.
/// When `showDragHandle` is set to `true`, then an iOS-style drag handle will be placed
/// at the top of the sheet. If a value is not provided, then it will default to
/// false.
|
Regarding bot comments, using viewInsets is intentional, so that content within a SafeArea will move out of the way of the grabber, but other content, like putting a close button inline with the grabber, can still be done. |
github-actions
Bot
removed
the
a: accessibility
|
Originally this PR wrapped the handle with the drag handle SemanticsRole. After talking with @chunhtai offline, that has been removed from this PR. The SemanticsRole calls out that tapping on the drag handle will trigger behavior that is normally triggered by the drag. Currently, nothing happens on tap for the drag handle. The native drag handle will trigger switching between detents, if those are used in the sheet, which is a feature we do not have yet #169832. Once that issue is added, we will want to conditionally wrap the handle with the SemanticsRole, depending on if detents are used or not. If there is no behavior on tap, we do not want to have a semantics label. For now, there is nothing to do. |
bda5575 to
40eb998
Compare
| /// means the sheet takes up only the bottom 10% of the screen. If not provided, defaults | ||
| /// to 0.08 (8% of screen height). | ||
| /// | ||
| /// When `showDragHandle` is set to `true`, then an iOS-style drag handle will |
There was a problem hiding this comment.
Isn't everything in this widget in iOS style?
| /// When `showDragHandle` is set to `true`, then an iOS-style drag handle will | |
| /// When `showDragHandle` is set to `true`, then a drag handle will |
| /// be placed at the top of the sheet. If a value is not provided, then it will | ||
| /// default to false. |
There was a problem hiding this comment.
| /// be placed at the top of the sheet. If a value is not provided, then it will | |
| /// default to false. | |
| /// be placed at the top of the sheet. This flag defaults to false. |
| /// Defaults to false. | ||
| final bool showDragHandle; | ||
|
|
||
| Widget _dragHandleBuilder(BuildContext context) { |
There was a problem hiding this comment.
Maybe
| Widget _dragHandleBuilder(BuildContext context) { | |
| Widget _sheetWithDragHandle(BuildContext context) { |
| return Stack( | ||
| children: <Widget>[ | ||
| MediaQuery( | ||
| data: MediaQuery.of(context).copyWith(viewInsets: const EdgeInsets.only(top: 30)), |
There was a problem hiding this comment.
Add a const for the 30 and maybe explain how you got it
40eb998 to
4eb9b00
Compare
|
Google failure looks unrelated |
4eb9b00 to
6279a42
Compare
MitchellGoodwin
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/179962, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
Bot
removed
the
autosubmit
6279a42 to
665f065
Compare
MitchellGoodwin
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
2 participants
Fixes #179358.
Adds the option to add a native styled drag handle to the top of a Cupertino sheet through the
showDragHandleproperty. By default this is false, the same way it is for the native SwiftUI sheet.Styling for the handle is based off of native versions of the handle, and Apple's Figma files.
From this PR:
From the iOS Maps app (this sheet has a slight transparency)
From the Figma files:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.