From c82c399eb720642bc9636b1d58ab25d00a468b15 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 12 Mar 2020 18:20:15 -0700 Subject: [PATCH] [DataTable] Hide arrow padding when not sorting (#51667) * Change onSort and add tests * Add doc * Regression test * Remove if (true) * Make test clearer * Clearer comment --- .../flutter/lib/src/material/data_table.dart | 45 ++++--- .../test/material/data_table_test.dart | 116 ++++++++++++++++++ 2 files changed, 143 insertions(+), 18 deletions(-) diff --git a/packages/flutter/lib/src/material/data_table.dart b/packages/flutter/lib/src/material/data_table.dart index 66e608c5001..7d54c354fa0 100644 --- a/packages/flutter/lib/src/material/data_table.dart +++ b/packages/flutter/lib/src/material/data_table.dart @@ -47,6 +47,10 @@ class DataColumn { /// [Icon] (typically using size 18), or a [Row] with an icon and /// some text. /// + /// By default, this widget will only occupy the minimal space. If you want + /// it to take the entire remaining space, e.g. when you want to use [Center], + /// you can wrap it with an [Expanded]. + /// /// The label should not include the sort indicator. final Widget label; @@ -516,18 +520,23 @@ class DataTable extends StatelessWidget { bool sorted, bool ascending, }) { - if (onSort != null) { - final Widget arrow = _SortArrow( - visible: sorted, - down: sorted ? ascending : null, - duration: _sortArrowAnimationDuration, - ); - const Widget arrowPadding = SizedBox(width: _sortArrowPadding); - label = Row( - textDirection: numeric ? TextDirection.rtl : null, - children: [ label, arrowPadding, arrow ], - ); + List arrowWithPadding() { + return onSort == null ? const [] : [ + _SortArrow( + visible: sorted, + down: sorted ? ascending : null, + duration: _sortArrowAnimationDuration, + ), + const SizedBox(width: _sortArrowPadding), + ]; } + label = Row( + textDirection: numeric ? TextDirection.rtl : null, + children: [ + label, + ...arrowWithPadding(), + ], + ); label = Container( padding: padding, height: headingRowHeight, @@ -553,12 +562,12 @@ class DataTable extends StatelessWidget { child: label, ); } - if (onSort != null) { - label = InkWell( - onTap: onSort, - child: label, - ); - } + // TODO(dkwingsmt): Only wrap Inkwell if onSort != null. Blocked by + // https://github.com/flutter/flutter/issues/51152 + label = InkWell( + onTap: onSort, + child: label, + ); return label; } @@ -702,7 +711,7 @@ class DataTable extends StatelessWidget { label: column.label, tooltip: column.tooltip, numeric: column.numeric, - onSort: () => column.onSort != null ? column.onSort(dataColumnIndex, sortColumnIndex != dataColumnIndex || !sortAscending) : null, + onSort: column.onSort != null ? () => column.onSort(dataColumnIndex, sortColumnIndex != dataColumnIndex || !sortAscending) : null, sorted: dataColumnIndex == sortColumnIndex, ascending: sortAscending, ); diff --git a/packages/flutter/test/material/data_table_test.dart b/packages/flutter/test/material/data_table_test.dart index c39b3322bd3..115287cec71 100644 --- a/packages/flutter/test/material/data_table_test.dart +++ b/packages/flutter/test/material/data_table_test.dart @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'package:flutter/gestures.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -918,4 +919,119 @@ void main() { boxDecoration = tableRow.decoration as BoxDecoration; expect(boxDecoration.border.bottom.width, thickness); }); + + testWidgets('DataTable column heading cell - with and without sorting', (WidgetTester tester) async { + Widget buildTable({ int sortColumnIndex, bool sortEnabled = true }) { + return DataTable( + sortColumnIndex: sortColumnIndex, + columns: [ + DataColumn( + label: const Expanded(child: Center(child: Text('Name'))), + tooltip: 'Name', + onSort: sortEnabled ? (_, __) {} : null, + ), + ], + rows: const [ + DataRow( + cells: [ + DataCell(Text('A long desert name')), + ], + ), + ] + ); + } + + // Start with without sorting + await tester.pumpWidget(MaterialApp( + home: Material(child: buildTable( + sortEnabled: false, + )), + )); + + { + final Finder nameText = find.text('Name'); + expect(nameText, findsOneWidget); + final Finder nameCell = find.ancestor(of: find.text('Name'), matching: find.byType(Container)).first; + expect(tester.getCenter(nameText), equals(tester.getCenter(nameCell))); + expect(find.descendant(of: nameCell, matching: find.byType(Icon)), findsNothing); + } + + // Turn on sorting + await tester.pumpWidget(MaterialApp( + home: Material(child: buildTable( + sortEnabled: true, + )), + )); + + { + final Finder nameText = find.text('Name'); + expect(nameText, findsOneWidget); + final Finder nameCell = find.ancestor(of: find.text('Name'), matching: find.byType(Container)).first; + expect(find.descendant(of: nameCell, matching: find.byType(Icon)), findsOneWidget); + } + + // Turn off sorting again + await tester.pumpWidget(MaterialApp( + home: Material(child: buildTable( + sortEnabled: false, + )), + )); + + { + final Finder nameText = find.text('Name'); + expect(nameText, findsOneWidget); + final Finder nameCell = find.ancestor(of: find.text('Name'), matching: find.byType(Container)).first; + expect(tester.getCenter(nameText), equals(tester.getCenter(nameCell))); + expect(find.descendant(of: nameCell, matching: find.byType(Icon)), findsNothing); + } + }); + + testWidgets('DataTable correctly renders with a mouse', (WidgetTester tester) async { + // Regression test for a bug described in + // https://github.com/flutter/flutter/pull/43735#issuecomment-589459947 + // Filed at https://github.com/flutter/flutter/issues/51152 + Widget buildTable({ int sortColumnIndex }) { + return DataTable( + sortColumnIndex: sortColumnIndex, + columns: [ + const DataColumn( + label: Expanded(child: Center(child: Text('column1'))), + tooltip: 'Column1', + ), + DataColumn( + label: const Expanded(child: Center(child: Text('column2'))), + tooltip: 'Column2', + onSort: (_, __) {}, + ), + ], + rows: const [ + DataRow( + cells: [ + DataCell(Text('Content1')), + DataCell(Text('Content2')), + ], + ), + ] + ); + } + + await tester.pumpWidget(MaterialApp( + home: Material(child: buildTable()), + )); + + expect(tester.renderObject(find.text('column1')).attached, true); + expect(tester.renderObject(find.text('column2')).attached, true); + + final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); + await gesture.addPointer(location: Offset.zero); + addTearDown(gesture.removePointer); + + await tester.pumpAndSettle(); + expect(tester.renderObject(find.text('column1')).attached, true); + expect(tester.renderObject(find.text('column2')).attached, true); + + // Wait for the tooltip timer to expire to prevent it scheduling a new frame + // after the view is destroyed, which causes exceptions. + await tester.pumpAndSettle(const Duration(seconds: 1)); + }); }