Add a bottom app bar to the floating action button motion demo.#16196
Add a bottom app bar to the floating action button motion demo.#16196DaveShuckerow merged 161 commits intoflutter:masterfrom
Conversation
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
This looks good and thanks for bringing it to the finish line!
Most of the feedback is just nit pickery.
| const _FabLocationConfiguration('End, undocked above the bottom app bar', _BabMode.END_FAB, FloatingActionButtonLocation.endFloat), | ||
| const _FabLocationConfiguration('Center, docked to the bottom app bar', _BabMode.CENTER_FAB, const _CenterDockedFloatingActionButtonLocation()), | ||
| const _FabLocationConfiguration('End, docked to the bottom app bar', _BabMode.END_FAB, const _EndDockedFloatingActionButtonLocation()), | ||
| const _FabLocationConfiguration('Start, docked to the top app bar', _BabMode.CENTER_FAB, const _TopStartFloatingActionButtonLocation()), |
There was a problem hiding this comment.
In the new version of the old demo (ha!) this location is called _StartTopFloatingActionButtonLocation()
There was a problem hiding this comment.
After that lands I'll sync.
| final List<_FabShapeConfiguration> _fabShapeConfigurations = <_FabShapeConfiguration>[ | ||
| const _FabShapeConfiguration('None', null), | ||
| new _FabShapeConfiguration('Circular', | ||
| new Builder(builder: (BuildContext context) { |
There was a problem hiding this comment.
It's a shame that this Build is needed (to create a context from which the Scaffold can be found).
It would be slightly preferable to give the Scaffold a global _scaffoldKey and have _showSnackbar() dispense with the context argument and _scaffoldKey.currentState.showSnackBar().
This is a common pattern in the gallery demos.
There was a problem hiding this comment.
Done, thanks for the suggestion.
| ); | ||
| } | ||
|
|
||
| Widget controls(BuildContext context) { |
There was a problem hiding this comment.
buildControls() for consistency.
| // The currently-selected Color for the Bottom App Bar. | ||
| Color babColor; | ||
|
|
||
| List<Color> babColors = <Color> [ |
| new Row( | ||
| mainAxisAlignment: MainAxisAlignment.spaceBetween, | ||
| children: <Widget>[ | ||
| new Container(width: 96.0, |
There was a problem hiding this comment.
width: on its own line
SizedBox would be a little tider than Container.
| } | ||
|
|
||
| // A bottom app bar with a menu inside it. | ||
| class MyBottomAppBar extends StatelessWidget { |
There was a problem hiding this comment.
For consistency, give this a _FooBottomAppBar name
| ); | ||
| } | ||
|
|
||
| Widget babContents(BuildContext context, _BabMode babMode) { |
| ), | ||
| ); | ||
| } | ||
| rowContents.addAll(<Widget> [ |
There was a problem hiding this comment.
This could be const <Widget> [ etc
There was a problem hiding this comment.
The widgets inside are newed because of their empty onPressed callbacks.
| } | ||
|
|
||
| // A drawer that pops up from the bottom of the screen. | ||
| class MyDrawer extends StatelessWidget { |
| return new Material( | ||
| shape: const _DiamondBorder(), | ||
| color: Colors.orange, | ||
| //onPressed: widget.onPressed, |
There was a problem hiding this comment.
Why is this commented out?
There was a problem hiding this comment.
Old code. Removed.
|
PTAL |
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
Just needs a few cleanups.
LGTM
| const _FabShapeConfiguration('None', null), | ||
| new _FabShapeConfiguration('Circular', | ||
| new FloatingActionButton( | ||
| onPressed: () => _showSnackbar(), |
There was a problem hiding this comment.
This can just be onPressed: _showSnackBar
There was a problem hiding this comment.
Tearoff applied
| ), | ||
| new _FabShapeConfiguration('Diamond', | ||
| new _DiamondFab( | ||
| onPressed: () => _showSnackbar(), |
There was a problem hiding this comment.
This can just be onPressed: _showSnackBar
There was a problem hiding this comment.
Tearoff applied
| value: color, | ||
| groupValue: babColor, | ||
| onChanged: (Color color) { setState(() { babColor = color; }); }, | ||
| ) |
There was a problem hiding this comment.
trailing comma (here and elsewhere)
| } | ||
| } | ||
|
|
||
| // TODO: Add support for making these docked configurations handle the Bottom App Bar being absent. |
There was a problem hiding this comment.
The docked classes shouldn't be here anymore
|
Has it been checked that the demo and the widgets involved are usable with TalkBack/VoiceOver? |
|
This demo has not been checked. |
|
Would be cool if that could be done as a follow-up and if potential issues could be fixed. |
|
Any code review going forward should verify all of the following for any widgets and demos we commit:
See https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement for up-to-date guidance. Let me know if there's anything about this that we can document better! Thanks. |
5 participants
@HansMuller here is the updated demo.