Implement resizeToAvoidBottomPadding in CupertinoPageScaffold #20929
Conversation
xster
left a comment
xster
left a comment
There was a problem hiding this comment.
Thanks for your addition. Please also add a test for this in test/cupertino/scaffold_test.dart. A mock MediaQuery with a viewInsets can be added above the scaffold.
| data: existingMediaQuery.copyWith( | ||
| padding: existingMediaQuery.padding.copyWith( | ||
| top: topPadding, | ||
| bottom: bottomPadding, |
There was a problem hiding this comment.
Ah my apologies, I misread your previous code. You weren't adding the MediaQuery.padding.bottom as padding, you were just propagating it down. That was correct.
This should just be existingMediaQuery.padding.bottom + bottomPadding as you had it before.
|
I had to wrap |
SPodjasek
changed the title
xster
left a comment
xster
left a comment
There was a problem hiding this comment.
Thanks for making the changes. I see what's happening with the tests. A few more comments. Please bear with me.
| @required this.child, | ||
| }) : assert(child != null), | ||
| super(key: key); | ||
| }) : assert(child != null), |
There was a problem hiding this comment.
Leave as before. There's 2 spaces now
| /// prevents widgets inside the body from being obscured by the keyboard. | ||
| /// | ||
| /// Defaults to true. | ||
| final bool resizeToAvoidBottomPadding; |
There was a problem hiding this comment.
Actually this is a good opportunity to make sure the API naming is current. Unfortunately the Material scaffold's name was created before we distinguished between full 'insets' where the usable view area shrinks due to things like keyboard and 'paddings' where it's obstructions from things like the home bar and notch etc.
Let's name this resizeToAvoidBottomInset
| /// By default uses [CupertinoColors.white] color. | ||
| final Color backgroundColor; | ||
|
|
||
| /// Whether the [child] should size itself to avoid the window's bottom padding. |
|
|
||
| // Propagate bottom padding and include viewInsets if appropriate | ||
| final double bottomPadding = resizeToAvoidBottomPadding | ||
| ? existingMediaQuery.viewInsets.bottom |
| data: existingMediaQuery.copyWith( | ||
| padding: existingMediaQuery.padding.copyWith( | ||
| top: topPadding, | ||
| bottom: existingMediaQuery.padding.bottom + bottomPadding, |
There was a problem hiding this comment.
Reading this a bit more carefully, I see what's happening in the tests. Instead of converting MediaQuery insets into MediaQuery paddings, we want to directly apply the insets to the contents.
In other words, leave the MediaQuery part as is but wrap that whole thing with a Padding with a bottom padding from the inset. This matches the Material scaffold's behavior more and you can remove the SafeAreas in the test.
xster
left a comment
xster
left a comment
There was a problem hiding this comment.
Cool, thanks for the changes! I'll merge it in when the build tree is green
| ), | ||
| ), | ||
| child: child, | ||
| child: Padding( |
There was a problem hiding this comment.
Sorry, the linter doesn't currently pop a warning for these but we're still using Dart1 syntax for the time being. new Padding here.
| this.backgroundColor = CupertinoColors.white, | ||
| this.resizeToAvoidBottomInset = true, | ||
| @required this.child, | ||
| }) : assert(child != null), |
There was a problem hiding this comment.
assert that resizeToAvoidBottomInset is not null like child here
3 participants
Currently CupertinoPageScaffold doesn't use viewInsets to pad content as Material Scaffold does.
You can see actual master behavior below and fixed version next to it.
Repository for above PoC app: https://github.com/SPodjasek/flutter_scaffold_poc