diff --git a/examples/flutter_gallery/lib/demo/icons_demo.dart b/examples/flutter_gallery/lib/demo/icons_demo.dart index b2e7b324833..a609adc253d 100644 --- a/examples/flutter_gallery/lib/demo/icons_demo.dart +++ b/examples/flutter_gallery/lib/demo/icons_demo.dart @@ -45,10 +45,10 @@ class IconsDemoState extends State { }); } - Widget buildIconButton(double size, IconData icon, bool enabled) { + Widget buildIconButton(double iconSize, IconData icon, bool enabled) { return new IconButton( icon: new Icon(icon), - size: size, + iconSize: iconSize, color: iconColor, tooltip: "${enabled ? 'Enabled' : 'Disabled'} icon button", onPressed: enabled ? handleIconButtonPress : null diff --git a/examples/flutter_gallery/lib/demo/tooltip_demo.dart b/examples/flutter_gallery/lib/demo/tooltip_demo.dart index 2e1dddd2521..164f49e9295 100644 --- a/examples/flutter_gallery/lib/demo/tooltip_demo.dart +++ b/examples/flutter_gallery/lib/demo/tooltip_demo.dart @@ -41,7 +41,7 @@ class TooltipDemo extends StatelessWidget { ), new Center( child: new IconButton( - size: 48.0, + iconSize: 48.0, icon: new Icon(Icons.call), color: theme.iconTheme.color, tooltip: 'Place a phone call', diff --git a/packages/flutter/lib/src/material/app_bar.dart b/packages/flutter/lib/src/material/app_bar.dart index c4b3e7fc4c6..3b29f5c49a7 100644 --- a/packages/flutter/lib/src/material/app_bar.dart +++ b/packages/flutter/lib/src/material/app_bar.dart @@ -46,8 +46,8 @@ class _ToolbarLayout extends MultiChildLayoutDelegate { // space bewteen the leading and actions widgets). final bool centerTitle; - static const double kLeadingWidth = 40.0; // Same size as an IconButton - static const double kTitleLeft = 64.0; // The AppBar pads left and right an additional 8.0. + static const double kLeadingWidth = 56.0; // So it's square with kToolbarHeight. + static const double kTitleLeft = 72.0; // As per https://material.io/guidelines/layout/metrics-keylines.html#metrics-keylines-keylines-spacing. @override void performLayout(Size size) { @@ -353,7 +353,6 @@ class _AppBarState extends State { if (_hasDrawer) { leading = new IconButton( icon: new Icon(Icons.menu), - alignment: FractionalOffset.centerLeft, onPressed: _handleDrawerButton, tooltip: 'Open navigation menu' // TODO(ianh): Figure out how to localize this string ); @@ -372,7 +371,6 @@ class _AppBarState extends State { assert(backIcon != null); leading = new IconButton( icon: new Icon(backIcon), - alignment: FractionalOffset.centerLeft, onPressed: _handleBackButton, tooltip: 'Back' // TODO(ianh): Figure out how to localize this string ); @@ -407,6 +405,7 @@ class _AppBarState extends State { id: _ToolbarSlot.actions, child: new Row( mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.stretch, children: config.actions, ), ), @@ -414,7 +413,7 @@ class _AppBarState extends State { } Widget toolbar = new Padding( - padding: const EdgeInsets.symmetric(horizontal: 8.0), + padding: const EdgeInsets.only(right: 4.0), child: new CustomMultiChildLayout( delegate: new _ToolbarLayout( centerTitle: config._getEffectiveCenterTitle(themeData), diff --git a/packages/flutter/lib/src/material/icon_button.dart b/packages/flutter/lib/src/material/icon_button.dart index de5024c5dcb..bb787a66bd2 100644 --- a/packages/flutter/lib/src/material/icon_button.dart +++ b/packages/flutter/lib/src/material/icon_button.dart @@ -17,6 +17,9 @@ import 'material.dart'; import 'theme.dart'; import 'tooltip.dart'; +// Minimum logical pixel size of the IconButton. +const double kMinButtonSize = 48.0; + /// A material design icon button. /// /// An icon button is a picture printed on a [Material] widget that reacts to @@ -30,6 +33,8 @@ import 'tooltip.dart'; /// /// Requires one of its ancestors to be a [Material] widget. /// +/// Will be automatically sized up to the recommended 48 logical pixels if smaller. +/// /// See also: /// /// * [Icons] @@ -42,14 +47,14 @@ class IconButton extends StatelessWidget { /// /// Requires one of its ancestors to be a [Material] widget. /// - /// The [size], [padding], and [alignment] arguments must not be null (though + /// The [iconSize], [padding], and [alignment] arguments must not be null (though /// they each have default values). /// /// The [icon] argument must be specified, and is typically either an [Icon] /// or an [ImageIcon]. const IconButton({ Key key, - this.size: 24.0, + this.iconSize: 24.0, this.padding: const EdgeInsets.all(8.0), this.alignment: FractionalOffset.center, @required this.icon, @@ -69,7 +74,7 @@ class IconButton extends StatelessWidget { /// fit the [Icon]. If you were to set the size of the [Icon] using /// [Icon.size] instead, then the [IconButton] would default to 24.0 and then /// the [Icon] itself would likely get clipped. - final double size; + final double iconSize; /// The padding around the button's icon. The entire padded icon will react /// to input gestures. @@ -136,29 +141,29 @@ class IconButton extends StatelessWidget { currentColor = color; else currentColor = disabledColor ?? Theme.of(context).disabledColor; - Widget result = new Padding( - padding: padding, - child: new LimitedBox( - maxWidth: size, - maxHeight: size, - child: new ConstrainedBox( - constraints: new BoxConstraints.loose( - new Size.square(math.max(size, Material.defaultSplashRadius * 2.0)) - ), + + Widget result = new ConstrainedBox( + constraints: new BoxConstraints(minWidth: kMinButtonSize, minHeight: kMinButtonSize), + child: new Padding( + padding: padding, + child: new SizedBox( + height: iconSize, + width: iconSize, child: new Align( alignment: alignment, child: new IconTheme.merge( context: context, data: new IconThemeData( - size: size, + size: iconSize, color: currentColor ), child: icon - ) - ) - ) - ) + ), + ), + ), + ), ); + if (tooltip != null) { result = new Tooltip( message: tooltip, @@ -168,7 +173,11 @@ class IconButton extends StatelessWidget { return new InkResponse( onTap: onPressed, child: result, - radius: math.max(size, Material.defaultSplashRadius), + radius: math.max( + Material.defaultSplashRadius, + (iconSize + math.min(padding.horizontal, padding.vertical)) * 0.7, + // x 0.5 for diameter -> radius and + 40% overflow derived from other Material apps. + ), ); } diff --git a/packages/flutter/test/material/app_bar_test.dart b/packages/flutter/test/material/app_bar_test.dart index f10ea8c1c2a..0d99a92b84a 100644 --- a/packages/flutter/test/material/app_bar_test.dart +++ b/packages/flutter/test/material/app_bar_test.dart @@ -108,8 +108,8 @@ void main() { Finder title = find.byKey(titleKey); expect(tester.getTopLeft(title).x, 72.0); - // The toolbar's contents are padded on the right by 8.0 - expect(tester.getSize(title).width, equals(800.0 - 72.0 - 8.0)); + // The toolbar's contents are padded on the right by 4.0 + expect(tester.getSize(title).width, equals(800.0 - 72.0 - 4.0)); actions = [ const SizedBox(width: 100.0), @@ -119,13 +119,13 @@ void main() { expect(tester.getTopLeft(title).x, 72.0); // The title shrinks by 200.0 to allow for the actions widgets. - expect(tester.getSize(title).width, equals(800.0 - 72.0 - 8.0 - 200.0)); + expect(tester.getSize(title).width, equals(800.0 - 72.0 - 4.0 - 200.0)); leading = new Container(); // AppBar will constrain the width to 24.0 await tester.pumpWidget(buildApp()); expect(tester.getTopLeft(title).x, 72.0); // Adding a leading widget shouldn't effect the title's size - expect(tester.getSize(title).width, equals(800.0 - 72.0 - 8.0 - 200.0)); + expect(tester.getSize(title).width, equals(800.0 - 72.0 - 4.0 - 200.0)); }); testWidgets('AppBar centerTitle:true title overflow OK ', (WidgetTester tester) async { @@ -165,18 +165,18 @@ void main() { // Centering a title with width 620 within the 800 pixel wide test widget // would mean that its left edge would have to be 90. We reserve 72 - // on the left and the padded actions occupy 90 + 8 on the right. That - // leaves 630, so the title is right justified but its width isn't changed. + // on the left and the padded actions occupy 96 + 4 on the right. That + // leaves 628, so the title is right justified but its width isn't changed. await tester.pumpWidget(buildApp()); leading = null; titleWidth = 620.0; actions = [ - const SizedBox(width: 45.0), - const SizedBox(width: 45.0) + const SizedBox(width: 48.0), + const SizedBox(width: 48.0) ]; await tester.pumpWidget(buildApp()); - expect(tester.getTopLeft(title).x, 800 - 620 - 45 - 45 - 8); + expect(tester.getTopLeft(title).x, 800 - 620 - 48 - 48 - 4); expect(tester.getSize(title).width, equals(620.0)); }); @@ -231,4 +231,59 @@ void main() { expect(yCenter(appBarKey), equals(yCenter(action1Key))); }); + testWidgets('leading button extends to edge and is square', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + theme: new ThemeData(platform: TargetPlatform.android), + home: new Scaffold( + appBar: new AppBar( + title: new Text('X'), + ), + drawer: new Column(), // Doesn't really matter. Triggers a hamburger regardless. + ) + ) + ); + + Finder hamburger = find.byTooltip('Open navigation menu'); + expect(tester.getTopLeft(hamburger), new Point(0.0, 0.0)); + expect(tester.getSize(hamburger), new Size(56.0, 56.0)); + }); + + testWidgets('test action is 4dp from edge and 48dp min', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + theme: new ThemeData(platform: TargetPlatform.android), + home: new Scaffold( + appBar: new AppBar( + title: new Text('X'), + actions: [ + new IconButton( + icon: new Icon(Icons.share), + onPressed: null, + tooltip: 'Share', + iconSize: 20.0, + ), + new IconButton( + icon: new Icon(Icons.add), + onPressed: null, + tooltip: 'Add', + iconSize: 60.0, + ), + ], + ), + ) + ) + ); + + Finder addButton = find.byTooltip('Add'); + // Right padding is 4dp. + expect(tester.getTopRight(addButton), new Point(800.0 - 4.0, 0.0)); + // It's still the size it was plus the 2 * 8dp padding from IconButton. + expect(tester.getSize(addButton), new Size(60.0 + 2 * 8.0, 56.0)); + + Finder shareButton = find.byTooltip('Share'); + // The 20dp icon is expanded to fill the IconButton's touch target to 48dp. + expect(tester.getSize(shareButton), new Size(48.0, 56.0)); + }); + } diff --git a/packages/flutter/test/material/icon_button_test.dart b/packages/flutter/test/material/icon_button_test.dart index 24cf7ff30b6..0e693e4e51c 100644 --- a/packages/flutter/test/material/icon_button_test.dart +++ b/packages/flutter/test/material/icon_button_test.dart @@ -5,26 +5,169 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; -void main() { - testWidgets('IconButton test constrained size', (WidgetTester tester) async { - const double kIconSize = 80.0; +class MockOnPressedFunction implements Function { + int called = 0; + void call() { + called++; + } +} + +void main() { + MockOnPressedFunction mockOnPressedFunction; + + setUp(() { + mockOnPressedFunction = new MockOnPressedFunction(); + }); + + testWidgets('test default icon buttons are sized up to 48', (WidgetTester tester) async { + await tester.pumpWidget( + new Material( + child: new Center( + child: new IconButton( + onPressed: mockOnPressedFunction, + icon: new Icon(Icons.link), + ), + ), + ), + ); + + RenderBox iconButton = tester.renderObject(find.byType(IconButton)); + expect(iconButton.size, new Size(48.0, 48.0)); + + await tester.tap(find.byType(IconButton)); + expect(mockOnPressedFunction.called, 1); + }); + + testWidgets('test small icons are sized up to 48dp', (WidgetTester tester) async { + await tester.pumpWidget( + new Material( + child: new Center( + child: new IconButton( + iconSize: 10.0, + onPressed: mockOnPressedFunction, + icon: new Icon(Icons.link), + ), + ), + ), + ); + + RenderBox iconButton = tester.renderObject(find.byType(IconButton)); + expect(iconButton.size, new Size(48.0, 48.0)); + }); + + testWidgets('test icons can be small when total size is >48dp', (WidgetTester tester) async { + await tester.pumpWidget( + new Material( + child: new Center( + child: new IconButton( + iconSize: 10.0, + padding: new EdgeInsets.all(30.0), + onPressed: mockOnPressedFunction, + icon: new Icon(Icons.link), + ), + ), + ), + ); + + RenderBox iconButton = tester.renderObject(find.byType(IconButton)); + expect(iconButton.size, new Size(70.0, 70.0)); + }); + + testWidgets('test default icon buttons are constrained', (WidgetTester tester) async { await tester.pumpWidget( new Material( child: new Center( child: new IconButton( padding: EdgeInsets.zero, - onPressed: () {}, + onPressed: mockOnPressedFunction, icon: new Icon(Icons.ac_unit), - size: kIconSize, - ) - ) - ) + iconSize: 80.0, + ), + ), + ), ); RenderBox box = tester.renderObject(find.byType(IconButton)); - expect(box.size.width, equals(kIconSize)); - expect(box.size.height, equals(kIconSize)); + expect(box.size, new Size(80.0, 80.0)); + }); + + testWidgets( + 'test default icon buttons can be stretched if specified', + (WidgetTester tester) async { + await tester.pumpWidget( + new Material( + child: new Row( + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + new IconButton( + onPressed: mockOnPressedFunction, + icon: new Icon(Icons.ac_unit), + ), + ], + ), + ), + ); + + RenderBox box = tester.renderObject(find.byType(IconButton)); + expect(box.size, new Size(48.0, 600.0)); + }); + + testWidgets('test default padding', (WidgetTester tester) async { + await tester.pumpWidget( + new Material( + child: new Center( + child: new IconButton( + onPressed: mockOnPressedFunction, + icon: new Icon(Icons.ac_unit), + iconSize: 80.0, + ), + ), + ), + ); + + RenderBox box = tester.renderObject(find.byType(IconButton)); + expect(box.size, new Size(96.0, 96.0)); + }); + + testWidgets('test tooltip', (WidgetTester tester) async { + await tester.pumpWidget( + new MaterialApp( + home: new Material( + child: new Center( + child: new IconButton( + onPressed: mockOnPressedFunction, + icon: new Icon(Icons.ac_unit), + ), + ), + ), + ), + ); + + expect(find.byType(Tooltip), findsNothing); + + // Clear the widget tree. + await tester.pumpWidget(new Container(key: new UniqueKey())); + + await tester.pumpWidget( + new MaterialApp( + home: new Material( + child: new Center( + child: new IconButton( + onPressed: mockOnPressedFunction, + icon: new Icon(Icons.ac_unit), + tooltip: 'Test tooltip', + ), + ), + ), + ), + ); + + expect(find.byType(Tooltip), findsOneWidget); + expect(find.byTooltip('Test tooltip'), findsOneWidget); + + await tester.tap(find.byTooltip('Test tooltip')); + expect(mockOnPressedFunction.called, 1); }); testWidgets('IconButton AppBar size', (WidgetTester tester) async { @@ -34,12 +177,12 @@ void main() { actions: [ new IconButton( padding: EdgeInsets.zero, - onPressed: () {}, + onPressed: mockOnPressedFunction, icon: new Icon(Icons.ac_unit), - ) - ] - ) - ) + ), + ], + ), + ), ); RenderBox barBox = tester.renderObject(find.byType(AppBar)); diff --git a/packages/flutter/test/material/paginated_data_table_test.dart b/packages/flutter/test/material/paginated_data_table_test.dart index 7bf7a9fdba5..98cc4e68597 100644 --- a/packages/flutter/test/material/paginated_data_table_test.dart +++ b/packages/flutter/test/material/paginated_data_table_test.dart @@ -42,6 +42,69 @@ class TestDataSource extends DataTableSource { } void main() { + testWidgets('PaginatedDataTable paging', (WidgetTester tester) async { + TestDataSource source = new TestDataSource(); + + List log = []; + + await tester.pumpWidget(new MaterialApp( + home: new PaginatedDataTable( + header: new Text('Test table'), + source: source, + rowsPerPage: 2, + availableRowsPerPage: [ + 2, 4, 8, 16, + ], + onRowsPerPageChanged: (int rowsPerPage) { + log.add('rows-per-page-changed: $rowsPerPage'); + }, + onPageChanged: (int rowIndex) { + log.add('page-changed: $rowIndex'); + }, + columns: [ + new DataColumn(label: new Text('Name')), + new DataColumn(label: new Text('Calories'), numeric: true), + new DataColumn(label: new Text('Generation')), + ], + ) + )); + + await tester.tap(find.byTooltip('Next page')); + + expect(log, ['page-changed: 2']); + log.clear(); + + await tester.pump(); + + expect(find.text('Frozen yogurt (0)'), findsNothing); + expect(find.text('Eclair (0)'), findsOneWidget); + expect(find.text('Gingerbread (0)'), findsNothing); + + await tester.tap(find.icon(Icons.chevron_left)); + + expect(log, ['page-changed: 0']); + log.clear(); + + await tester.pump(); + + expect(find.text('Frozen yogurt (0)'), findsOneWidget); + expect(find.text('Eclair (0)'), findsNothing); + expect(find.text('Gingerbread (0)'), findsNothing); + + await tester.tap(find.icon(Icons.chevron_left)); + + expect(log, isEmpty); + + await tester.tap(find.text('2')); + await tester.pumpUntilNoTransientCallbacks(const Duration(milliseconds: 200)); + + await tester.tap(find.text('8').last); + await tester.pumpUntilNoTransientCallbacks(const Duration(milliseconds: 200)); + + expect(log, ['rows-per-page-changed: 8']); + log.clear(); + }); + testWidgets('PaginatedDataTable control test', (WidgetTester tester) async { TestDataSource source = new TestDataSource() ..generation = 42; @@ -126,67 +189,4 @@ void main() { expect(log, ['action: adjust']); log.clear(); }); - - testWidgets('PaginatedDataTable paging', (WidgetTester tester) async { - TestDataSource source = new TestDataSource(); - - List log = []; - - await tester.pumpWidget(new MaterialApp( - home: new PaginatedDataTable( - header: new Text('Test table'), - source: source, - rowsPerPage: 2, - availableRowsPerPage: [ - 2, 4, 8, 16, - ], - onRowsPerPageChanged: (int rowsPerPage) { - log.add('rows-per-page-changed: $rowsPerPage'); - }, - onPageChanged: (int rowIndex) { - log.add('page-changed: $rowIndex'); - }, - columns: [ - new DataColumn(label: new Text('Name')), - new DataColumn(label: new Text('Calories'), numeric: true), - new DataColumn(label: new Text('Generation')), - ], - ) - )); - - await tester.tap(find.byTooltip('Next page')); - - expect(log, ['page-changed: 2']); - log.clear(); - - await tester.pump(); - - expect(find.text('Frozen yogurt (0)'), findsNothing); - expect(find.text('Eclair (0)'), findsOneWidget); - expect(find.text('Gingerbread (0)'), findsNothing); - - await tester.tap(find.icon(Icons.chevron_left)); - - expect(log, ['page-changed: 0']); - log.clear(); - - await tester.pump(); - - expect(find.text('Frozen yogurt (0)'), findsOneWidget); - expect(find.text('Eclair (0)'), findsNothing); - expect(find.text('Gingerbread (0)'), findsNothing); - - await tester.tap(find.icon(Icons.chevron_left)); - - expect(log, isEmpty); - - await tester.tap(find.text('2')); - await tester.pumpUntilNoTransientCallbacks(const Duration(milliseconds: 200)); - - await tester.tap(find.text('8').last); - await tester.pumpUntilNoTransientCallbacks(const Duration(milliseconds: 200)); - - expect(log, ['rows-per-page-changed: 8']); - log.clear(); - }); }