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. |
MitchellGoodwin
force-pushed
the
sheet-grabber
branch
from
bda5575 to
40eb998
Compare
1 participant
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.