Apply media bottom padding in BottomNavigationBar #13431
Conversation
| @@ -930,6 +933,7 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin { | |||
| removeLeftPadding: false, | |||
| removeTopPadding: true, | |||
| removeRightPadding: false, | |||
| removeBottomPadding: false, | |||
There was a problem hiding this comment.
bah, github doesn't let me comment on unchanged lines. You'd want to remove the bottom padding from the body now no? Since it's 'consumed'.
There was a problem hiding this comment.
The body already does remove the bottom padding if necessary (line 861). Bottom padding removal is defaulted to widget.resizeToAvoidBottomPadding in _addIfNonNull() (lines 812-833).
Once we've moved fully to viewInsets on the engine side that'll go away and, I'll make that a @required param to make the call sites clearer.
There was a problem hiding this comment.
If in the ultimate future, resizeToAvoidBottomPadding is gone, don't we still need to explicitly set removebottompadding to true if bottom tab bar != null?
There was a problem hiding this comment.
In the future, the padding is the safe area padding so we don't want to remove it, we want the nav bar to adapt itself for the additional padding.
| @@ -436,6 +436,8 @@ class _BottomNavigationBarState extends State<BottomNavigationBar> with TickerPr | |||
| @override | |||
| Widget build(BuildContext context) { | |||
| assert(debugCheckHasDirectionality(context)); | |||
|
|
|||
| final double bottomPadding = math.max(MediaQuery.of(context).padding.bottom - _kBottomMargin, 0.0); | |||
There was a problem hiding this comment.
FWIW: not entirely thrilled with this. An alternative would be to deal with this in the individual item widgets where we deal with bottom margin. It's not much less convoluted to do it in those two places though, and applying it as a const bottom padding results in an easier-to-follow tree. Ideas welcome :)
There was a problem hiding this comment.
I think consolidating here is fine. Though some comment / const renaming might help future readers. Something like we only need to shift up if the partial obstruction may intersect with interactive parts of this widget, liberally generalized by this const (feel free to do all this in a different PR).
There was a problem hiding this comment.
Maybe the 2 names needs some additional differentiation too. They're both called bottom padding which is a bit confusing when one is a outer container padding and one is an inner layout padding.
There was a problem hiding this comment.
Good point. I'll update the names.
cbracken
force-pushed
the
iphone_x-bottom-nav-bar
branch
from
de1fd26
to
05f1875
Compare
| ); | ||
|
|
||
| const double labelBottomMargin = 8.0; // _kBottomMargin in implementation. | ||
| const double additionalPadding = 40.0 - labelBottomMargin; |
There was a problem hiding this comment.
<not a real comment>Ha, you even called this 'additional' padding here :)</>
There was a problem hiding this comment.
Stole the name and used it in place of bottomPadding internally.
|
LGTM, feel free to respond/address everything in a separate PR if this unblocks you |
cbracken
force-pushed
the
iphone_x-bottom-nav-bar
branch
from
05f1875
to
2d30a76
Compare
|
Switching over to viewInsets and making |
Updates scaffold to expose bottom padding to the associated BottonNavigationBar (if present). BottomNavigationBar is now responsible for adjusting itself to account for bottom padding. This change is necessary to support the updated BottomNavigationBar layout for the iPhone X.
cbracken
force-pushed
the
iphone_x-bottom-nav-bar
branch
from
2d30a76
to
03f80ea
Compare
Updates scaffold to expose bottom padding to the associated BottonNavigationBar (if present). BottomNavigationBar is now responsible for adjusting itself to account for bottom padding. This change is necessary to support the updated BottomNavigationBar layout for the iPhone X.
Updates scaffold to expose bottom padding to the associated
BottonNavigationBar (if present). BottomNavigationBar is now responsible
for adjusting itself to account for bottom padding.
This change is necessary to support the updated BottomNavigationBar
layout for the iPhone X.
Fixes #12099 once the associated engine roll lands.