- Notifications
You must be signed in to change notification settings - Fork30k
Adjust padding for Cupertino sheet content#162481
Adjust padding for Cupertino sheet content#162481auto-submit[bot] merged 2 commits intoflutter:masterfrom
Conversation
justinmc left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM with nits 👍
Looks like you have an analyzer failure FYI.
| child: _CupertinoSheetScope(child: builder(context)), | ||
| final double bottomPadding = MediaQuery.sizeOf(context).height * _kTopGapRatio; | ||
| return MediaQuery.removePadding( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I didn't realize MediaQuery.removePadding was a thing, nice.
| // of the total height of the screen. Found from eyeballing a simulator running | ||
| // iOS 18.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: Have you tried this PR out on iPad? If not and you have time maybe give it a shot, just to double check that this proportion still looks ok on the bigger screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It looks ok. Natively sheets adapt to a modal type view though. So there's nothing to compare against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh I didn't realize that. Should we consider doing something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll create an issue for it, but I think it's going to be p3.
| // Devide by size of the sheet. The gap between the top of the sheet and | ||
| // top of the screen is 0.08. | ||
| details.primaryDelta! / (context.size!.height - (context.size!.height * topGapRatio)), | ||
| // Devide by size of the sheet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: "Divide"
| await tester.tap(find.text('Push Page 2')); | ||
| await tester.pumpAndSettle(); | ||
| expect(tester.getSize(find.byType(Container)).height, 600.0 - (600.0 * 0.08)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: Maybe declare a _kTopGapRatio variable in the test and use it here, just so it's more obvious where the 0.08 comes from.
fab2553 to10cf540Compare10cf540 to9cd9037Compareccae8ccUh oh!
There was an error while loading.Please reload this page.
muttsu-623 commentedMar 14, 2025
Hi@MitchellGoodwin ! I've met this issue. I appreciate your work! Thank you so much ❤️ Do you know when this fix will be released 👀 ? |
MitchellGoodwin commentedApr 11, 2025
Apologies, I missed this comment. This is currently on the beta build and will be a part of the next stable release. Stable release dates aren't public... But they can be guessed at by looking around 😄 |
Fixes#162215 and partially supports#162181.
The Cupertino sheet had two issues layout: It still had MediaQuery padding at the top of the screen as if there was safe area content to avoid and some of the sheet was permanently offscreen. This caused a FloatingActionButton to be clipped, but if you put a scrolling widget in the sheet with text at the very bottom, it was impossible to scroll the text into view. This PR removes the top padding, and changes the bottom padding to equal the amount offscreen.
Before:
After:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel onDiscord.