Repository navigation
Forward showDragHandle from showCupertinoSheet to CupertinoSheetRoute - #187803
theprantadutta wants to merge 2 commits into
Conversation
showCupertinoSheet accepts a showDragHandle parameter but never passed it to the CupertinoSheetRoute it constructs, in either the plain or the useNestedNavigation branch. Since CupertinoSheetRoute.showDragHandle defaults to false, the value was silently dropped and no drag handle was rendered, even though pushing CupertinoSheetRoute directly with the same flag shows one. Fixes flutter#187693
![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 forwards the showDragHandle parameter to the sheet route in showCupertinoSheet and adds a widget test to verify this behavior. Feedback suggests simplifying the scrollableBuilder callback in the new test by using the (_, __) shorthand for unused parameters to make the code more concise.
| scrollableBuilder: | ||
| (BuildContext context, ScrollController scrollController) { | ||
| return const CupertinoPageScaffold(child: Text('Page 2')); | ||
| }, |
There was a problem hiding this comment.
The context and scrollController parameters in scrollableBuilder are unused. We can simplify this callback using the (_, __) shorthand to make the code more concise and idiomatic.
| scrollableBuilder: | |
| (BuildContext context, ScrollController scrollController) { | |
| return const CupertinoPageScaffold(child: Text('Page 2')); | |
| }, | |
| scrollableBuilder: (_, __) => const CupertinoPageScaffold(child: Text('Page 2')), |
|
Regarding the code freeze check: this forwards a public parameter ( |
github-actions
Bot
added
framework
Piinks
added
the
Decoupling: Not ready to port yet
|
I've marked this PR as not ready to port to flutter/packages yet. |
|
Thanks for the reviews @dkwingsmt @navaronbracke! All the functional CI is green and this is approved — the only remaining red is the |
@theprantadutta See above :) We are currently taking inventory of all the PR's that do things for material_ui/cupertino_ui, so that we can orchestrate the move to said packages, when the code freeze is lifted. |
|
Thanks @navaronbracke, that makes total sense — no rush at all. Happy to port it over to |
dkwingsmt
added
the
waiting for code freeze
|
If you would like to return to this, material_ui and cupertino_ui are now accepting contributions. :) |
dkwingsmt
added
Decoupling: Port to flutter/packages
|
Thanks @dkwingsmt — ported to |
…rtinoSheetRoute (#13011) Port of flutter/flutter#187803 to `cupertino_ui`, following the process in flutter/flutter#188444. The original PR was approved by dkwingsmt and navaronbracke but couldn't land because of the Material/Cupertino code freeze. `showCupertinoSheet` accepts a `showDragHandle` parameter and documents what it does, but never passes it to the `CupertinoSheetRoute` it builds — so setting it to `true` has no effect. This forwards it on both paths (the plain route and the nested-navigation route). The regression test calls `showCupertinoSheet(showDragHandle: true)` and checks the drag handle is actually rendered. I checked that it fails with the forwarding removed and passes with it, in its new location under `packages/cupertino_ui/test/`. The original commit was cherry-picked as the porting guide describes. It hit one conflict: since the copy, `main` added `hasPlatformViews: hasPlatformViews` to the same two `CupertinoSheetRoute` constructor calls. I resolved it by keeping both arguments; the bug itself is still present on `main` (`showDragHandle` is declared but never forwarded). The only new commit adds the pending changelog entry (`version: patch`). Fixes flutter/flutter#187693 ## Pre-Review Checklist I used AI tooling while working on this, and reviewed and tested all of it myself. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
4 participants
showCupertinoSheetaccepts ashowDragHandleparameter but never passed it to theCupertinoSheetRouteit constructs — in either the plain branch or theuseNestedNavigationbranch. SinceCupertinoSheetRoute.showDragHandledefaults tofalse, the value was silently dropped and no drag handle was rendered, even though pushingCupertinoSheetRoutedirectly with the same flag shows one.This PR forwards the parameter in both branches and adds a regression test that opens a sheet through
showCupertinoSheetwithshowDragHandle: true— once withuseNestedNavigation: falseand once withtrue— and asserts the drag handle is rendered, using the same drag-handle finder as the existingCupertinoSheetRoute.showDragHandletests. The test fails without the fix and passes with it; the fullsheet_test.dartsuite (38 tests) passes.The drag handle being invisible above stacked sheets (#187697) is a separate issue and intentionally out of scope here.
Fixes #187693
Pre-launch Checklist
///).