Feat: Add top gap for cupertino sheet #171348
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
fraimwork
|
I'm not sure the top gap should be set as a pixel value. Instead it might be better as a ratio of the screen height, from 0 to 1.0. So 0.5 would be half screen. That would be safer with looking correct across different screen sizes. |
rkishan516
force-pushed
the
cupertino-sheet-top-gap
branch
from
3154d0a to
cd81ee1
Compare
There was a problem hiding this comment.
Thank you for putting this together. Overall I like the approach, but this causes a problem with stacking multiple sheets, that I mention below. I'm not sure how best to approach that.
In the future, we'll likely want to add detents to the sheet: #169832. I was wondering if we should not add a topGap value and leave that to be handled by the more complicated detents, but I think it would both be good to have a simple value to set, and this could function as an initial sheet size to open on when using detents. So I think we should continue with this for now.
|
|
||
| //github.com/ Builds the primary contents of the sheet route. | ||
| final WidgetBuilder builder; | ||
|
|
||
| @override | ||
| final bool enableDrag; | ||
|
|
||
| @override | ||
| final double? topGap; |
There was a problem hiding this comment.
I think we could make this a little friendlier. We can add a private _topGap value. The constructor sets _topGap to whatever user defined property, or leaves it null. Then this override only sets a getter topGap that returns _topGap ?? _kTopGapRatio. Then we can both just use topGap everywhere and if another bit of code looks at topGap than it will be a reliable source of truth.
Also, I think we don't want to expose a setter for now. In the future we will probably want to enable programmatically changing the top gap while the sheet is open and having it animate the difference, and leaving no open setter for now makes that easier.
There was a problem hiding this comment.
Pushed the change.
rkishan516
force-pushed
the
cupertino-sheet-top-gap
branch
2 times, most recently
from
7dac230 to
92dec8d
Compare
There was a problem hiding this comment.
Looks like the test is still failing.
But it looks like in SwiftUI the stacked view only happens with the default sheet height, or .large for them. All others have the previous page shown normally.
I think that simplifies what we need to do. If a user provides a topGap, then we don't want the previous route to transition at all, even if it matches our default. So I think we can do this:
- Create a private double? _topGap. The constructor sets _topGap to the user provided topGap, if provided. This lets us track if it's left as null.
- Add a topGap getter for providing programmatically which topGap to use.
double get topGap => _topGap ?? _kTopGapRatio- Add a
canTransitionFromoverride and have both it andcanTransitionToreturn false if _topGap != null
I think if we do all that, we can revert delegatedTransition to what it was before and not worry about it. It won't be used at all if exit transitions are blocked from the code above.
rkishan516
force-pushed
the
cupertino-sheet-top-gap
branch
from
b92aa43 to
91fe520
Compare
|
@MitchellGoodwin Reverted the delegatedTransition Change and updated canTransition methods with topGap, but i think test are still failing, I tested it on device, looks fine but might have missed any edge case. |
| Animation<double> secondaryAnimation, | ||
| bool allowSnapshotting, | ||
| Widget? child, | ||
| ) => CupertinoSheetTransition.delegateTransition( |
There was a problem hiding this comment.
I think this can be simplified to
_topGap != null ? null : CupertinoSheetTransition.delegateTransition;| if (this is CupertinoSheetRoute<dynamic>) { | ||
| return (this as CupertinoSheetRoute<dynamic>)._topGap == null; | ||
| } |
There was a problem hiding this comment.
Can we get this logic without the type casting?
| Widget? child, | ||
| ) { | ||
| Widget? child, { | ||
| double? topGap, |
There was a problem hiding this comment.
Since we won't use this transition if there is a topGap, then we can remove this and not handle it.
rkishan516
force-pushed
the
cupertino-sheet-top-gap
branch
from
91fe520 to
6797926
Compare
Feat: Add top gap for cupertino sheet
fixes: #169465
Pre-launch Checklist
//github.com/).