Unverified Commit 3681b27a authored by David Neuy's avatar David Neuy Committed by GitHub

Fix DataCell overflows when cell height is large by adding dataRowMinHeight,...

Fix DataCell overflows when cell height is large by adding dataRowMinHeight, dataRowMaxHeight props. (#114338)

* Fix DataCell overflows when cell height is large by adding dataRowMinHeight, dataRowMaxHeight props.

* Fix DataCell overflows when cell height is large by adding dataRowMinHeight, dataRowMaxHeight props - add tests.

* Fix analysis errors

* Review changes.

* Add asserts for dataRowMinHeight and dataRowMaxHeight

* Add asserts for dataRowMinHeight and dataRowMaxHeight

* Make dataRowHeight a computed getter

* Remove get only dataRowHeight from hashCode...

* Update deprecated after

* Add new line at end of AUTHORS

* Apply suggestions from code review

* Update packages/flutter/test/material/data_table_test.dart

---------
Co-authored-by: 's avatarKate Lovett <katelovett@google.com>
parent 97938859
......@@ -101,5 +101,6 @@ Junhua Lin <1075209054@qq.com>
Tomasz Gucio <tgucio@gmail.com>
Jason C.H <ctrysbita@outlook.com>
Hubert Jóźwiak <hjozwiakdx@gmail.com>
David Neuy <quantjump@gmail.com>
Eli Albert <crasowas@gmail.com>
Jan Kuß <jan@kuss.dev>
......@@ -392,7 +392,13 @@ class DataTable extends StatelessWidget {
this.onSelectAll,
this.decoration,
this.dataRowColor,
this.dataRowHeight,
@Deprecated(
'Migrate to use dataRowMinHeight and dataRowMaxHeight instead. '
'This feature was deprecated after v3.7.0-5.0.pre.',
)
double? dataRowHeight,
double? dataRowMinHeight,
double? dataRowMaxHeight,
this.dataTextStyle,
this.headingRowColor,
this.headingRowHeight,
......@@ -410,6 +416,11 @@ class DataTable extends StatelessWidget {
assert(sortColumnIndex == null || (sortColumnIndex >= 0 && sortColumnIndex < columns.length)),
assert(!rows.any((DataRow row) => row.cells.length != columns.length)),
assert(dividerThickness == null || dividerThickness >= 0),
assert(dataRowMinHeight == null || dataRowMaxHeight == null || dataRowMaxHeight >= dataRowMinHeight),
assert(dataRowHeight == null || (dataRowMinHeight == null && dataRowMaxHeight == null),
'dataRowHeight ($dataRowHeight) must not be set if dataRowMinHeight ($dataRowMinHeight) or dataRowMaxHeight ($dataRowMaxHeight) are set.'),
dataRowMinHeight = dataRowHeight ?? dataRowMinHeight,
dataRowMaxHeight = dataRowHeight ?? dataRowMaxHeight,
_onlyTextColumn = _initOnlyTextColumn(columns);
/// The configuration and labels for the columns in the table.
......@@ -504,7 +515,29 @@ class DataTable extends StatelessWidget {
/// If null, [DataTableThemeData.dataRowHeight] is used. This value defaults
/// to [kMinInteractiveDimension] to adhere to the Material Design
/// specifications.
final double? dataRowHeight;
@Deprecated(
'Migrate to use dataRowMinHeight and dataRowMaxHeight instead. '
'This feature was deprecated after v3.7.0-5.0.pre.',
)
double? get dataRowHeight => dataRowMinHeight == dataRowMaxHeight ? dataRowMinHeight : null;
/// {@template flutter.material.dataTable.dataRowMinHeight}
/// The minimum height of each row (excluding the row that contains column headings).
/// {@endtemplate}
///
/// If null, [DataTableThemeData.dataRowMinHeight] is used. This value defaults
/// to [kMinInteractiveDimension] to adhere to the Material Design
/// specifications.
final double? dataRowMinHeight;
/// {@template flutter.material.dataTable.dataRowMaxHeight}
/// The maximum height of each row (excluding the row that contains column headings).
/// {@endtemplate}
///
/// If null, [DataTableThemeData.dataRowMaxHeight] is used. This value defaults
/// to [kMinInteractiveDimension] to adhere to the Material Design
/// specifications.
final double? dataRowMaxHeight;
/// {@template flutter.material.dataTable.dataTextStyle}
/// The text style for data rows.
......@@ -841,13 +874,17 @@ class DataTable extends StatelessWidget {
?? dataTableTheme.dataTextStyle
?? themeData.dataTableTheme.dataTextStyle
?? themeData.textTheme.bodyMedium!;
final double effectiveDataRowHeight = dataRowHeight
?? dataTableTheme.dataRowHeight
?? themeData.dataTableTheme.dataRowHeight
final double effectiveDataRowMinHeight = dataRowMinHeight
?? dataTableTheme.dataRowMinHeight
?? themeData.dataTableTheme.dataRowMinHeight
?? kMinInteractiveDimension;
final double effectiveDataRowMaxHeight = dataRowMaxHeight
?? dataTableTheme.dataRowMaxHeight
?? themeData.dataTableTheme.dataRowMaxHeight
?? kMinInteractiveDimension;
label = Container(
padding: padding,
height: effectiveDataRowHeight,
constraints: BoxConstraints(minHeight: effectiveDataRowMinHeight, maxHeight: effectiveDataRowMaxHeight),
alignment: numeric ? Alignment.centerRight : AlignmentDirectional.centerStart,
child: DefaultTextStyle(
style: effectiveDataTextStyle.copyWith(
......@@ -1063,6 +1100,7 @@ class DataTable extends StatelessWidget {
clipBehavior: clipBehavior,
child: Table(
columnWidths: tableColumns.asMap(),
defaultVerticalAlignment: TableCellVerticalAlignment.middle,
children: tableRows,
border: border,
),
......
......@@ -40,7 +40,13 @@ class DataTableThemeData with Diagnosticable {
const DataTableThemeData({
this.decoration,
this.dataRowColor,
this.dataRowHeight,
@Deprecated(
'Migrate to use dataRowMinHeight and dataRowMaxHeight instead. '
'This feature was deprecated after v3.7.0-5.0.pre.',
)
double? dataRowHeight,
double? dataRowMinHeight,
double? dataRowMaxHeight,
this.dataTextStyle,
this.headingRowColor,
this.headingRowHeight,
......@@ -49,7 +55,11 @@ class DataTableThemeData with Diagnosticable {
this.columnSpacing,
this.dividerThickness,
this.checkboxHorizontalMargin,
});
}) : assert(dataRowMinHeight == null || dataRowMaxHeight == null || dataRowMaxHeight >= dataRowMinHeight),
assert(dataRowHeight == null || (dataRowMinHeight == null && dataRowMaxHeight == null),
'dataRowHeight ($dataRowHeight) must not be set if dataRowMinHeight ($dataRowMinHeight) or dataRowMaxHeight ($dataRowMaxHeight) are set.'),
dataRowMinHeight = dataRowHeight ?? dataRowMinHeight,
dataRowMaxHeight = dataRowHeight ?? dataRowMaxHeight;
/// {@macro flutter.material.dataTable.decoration}
final Decoration? decoration;
......@@ -59,7 +69,17 @@ class DataTableThemeData with Diagnosticable {
final MaterialStateProperty<Color?>? dataRowColor;
/// {@macro flutter.material.dataTable.dataRowHeight}
final double? dataRowHeight;
@Deprecated(
'Migrate to use dataRowMinHeight and dataRowMaxHeight instead. '
'This feature was deprecated after v3.7.0-5.0.pre.',
)
double? get dataRowHeight => dataRowMinHeight == dataRowMaxHeight ? dataRowMinHeight : null;
/// {@macro flutter.material.dataTable.dataRowMinHeight}
final double? dataRowMinHeight;
/// {@macro flutter.material.dataTable.dataRowMaxHeight}
final double? dataRowMaxHeight;
/// {@macro flutter.material.dataTable.dataTextStyle}
final TextStyle? dataTextStyle;
......@@ -91,7 +111,13 @@ class DataTableThemeData with Diagnosticable {
DataTableThemeData copyWith({
Decoration? decoration,
MaterialStateProperty<Color?>? dataRowColor,
@Deprecated(
'Migrate to use dataRowMinHeight and dataRowMaxHeight instead. '
'This feature was deprecated after v3.7.0-5.0.pre.',
)
double? dataRowHeight,
double? dataRowMinHeight,
double? dataRowMaxHeight,
TextStyle? dataTextStyle,
MaterialStateProperty<Color?>? headingRowColor,
double? headingRowHeight,
......@@ -105,6 +131,8 @@ class DataTableThemeData with Diagnosticable {
decoration: decoration ?? this.decoration,
dataRowColor: dataRowColor ?? this.dataRowColor,
dataRowHeight: dataRowHeight ?? this.dataRowHeight,
dataRowMinHeight: dataRowMinHeight ?? this.dataRowMinHeight,
dataRowMaxHeight: dataRowMaxHeight ?? this.dataRowMaxHeight,
dataTextStyle: dataTextStyle ?? this.dataTextStyle,
headingRowColor: headingRowColor ?? this.headingRowColor,
headingRowHeight: headingRowHeight ?? this.headingRowHeight,
......@@ -128,7 +156,8 @@ class DataTableThemeData with Diagnosticable {
return DataTableThemeData(
decoration: Decoration.lerp(a.decoration, b.decoration, t),
dataRowColor: MaterialStateProperty.lerp<Color?>(a.dataRowColor, b.dataRowColor, t, Color.lerp),
dataRowHeight: lerpDouble(a.dataRowHeight, b.dataRowHeight, t),
dataRowMinHeight: lerpDouble(a.dataRowMinHeight, b.dataRowMinHeight, t),
dataRowMaxHeight: lerpDouble(a.dataRowMaxHeight, b.dataRowMaxHeight, t),
dataTextStyle: TextStyle.lerp(a.dataTextStyle, b.dataTextStyle, t),
headingRowColor: MaterialStateProperty.lerp<Color?>(a.headingRowColor, b.headingRowColor, t, Color.lerp),
headingRowHeight: lerpDouble(a.headingRowHeight, b.headingRowHeight, t),
......@@ -144,7 +173,8 @@ class DataTableThemeData with Diagnosticable {
int get hashCode => Object.hash(
decoration,
dataRowColor,
dataRowHeight,
dataRowMinHeight,
dataRowMaxHeight,
dataTextStyle,
headingRowColor,
headingRowHeight,
......@@ -166,7 +196,8 @@ class DataTableThemeData with Diagnosticable {
return other is DataTableThemeData
&& other.decoration == decoration
&& other.dataRowColor == dataRowColor
&& other.dataRowHeight == dataRowHeight
&& other.dataRowMinHeight == dataRowMinHeight
&& other.dataRowMaxHeight == dataRowMaxHeight
&& other.dataTextStyle == dataTextStyle
&& other.headingRowColor == headingRowColor
&& other.headingRowHeight == headingRowHeight
......@@ -182,7 +213,8 @@ class DataTableThemeData with Diagnosticable {
super.debugFillProperties(properties);
properties.add(DiagnosticsProperty<Decoration>('decoration', decoration, defaultValue: null));
properties.add(DiagnosticsProperty<MaterialStateProperty<Color?>>('dataRowColor', dataRowColor, defaultValue: null));
properties.add(DoubleProperty('dataRowHeight', dataRowHeight, defaultValue: null));
properties.add(DoubleProperty('dataRowMinHeight', dataRowMinHeight, defaultValue: null));
properties.add(DoubleProperty('dataRowMaxHeight', dataRowMaxHeight, defaultValue: null));
properties.add(DiagnosticsProperty<TextStyle>('dataTextStyle', dataTextStyle, defaultValue: null));
properties.add(DiagnosticsProperty<MaterialStateProperty<Color?>>('headingRowColor', headingRowColor, defaultValue: null));
properties.add(DoubleProperty('headingRowHeight', headingRowHeight, defaultValue: null));
......
......@@ -71,7 +71,13 @@ class PaginatedDataTable extends StatefulWidget {
this.sortColumnIndex,
this.sortAscending = true,
this.onSelectAll,
this.dataRowHeight = kMinInteractiveDimension,
@Deprecated(
'Migrate to use dataRowMinHeight and dataRowMaxHeight instead. '
'This feature was deprecated after v3.7.0-5.0.pre.',
)
double? dataRowHeight,
double? dataRowMinHeight,
double? dataRowMaxHeight,
this.headingRowHeight = 56.0,
this.horizontalMargin = 24.0,
this.columnSpacing = 56.0,
......@@ -91,6 +97,11 @@ class PaginatedDataTable extends StatefulWidget {
}) : assert(actions == null || (header != null)),
assert(columns.isNotEmpty),
assert(sortColumnIndex == null || (sortColumnIndex >= 0 && sortColumnIndex < columns.length)),
assert(dataRowMinHeight == null || dataRowMaxHeight == null || dataRowMaxHeight >= dataRowMinHeight),
assert(dataRowHeight == null || (dataRowMinHeight == null && dataRowMaxHeight == null),
'dataRowHeight ($dataRowHeight) must not be set if dataRowMinHeight ($dataRowMinHeight) or dataRowMaxHeight ($dataRowMaxHeight) are set.'),
dataRowMinHeight = dataRowHeight ?? dataRowMinHeight ?? kMinInteractiveDimension,
dataRowMaxHeight = dataRowHeight ?? dataRowMaxHeight ?? kMinInteractiveDimension,
assert(rowsPerPage > 0),
assert(() {
if (onRowsPerPageChanged != null) {
......@@ -147,7 +158,23 @@ class PaginatedDataTable extends StatefulWidget {
///
/// This value is optional and defaults to kMinInteractiveDimension if not
/// specified.
final double dataRowHeight;
@Deprecated(
'Migrate to use dataRowMinHeight and dataRowMaxHeight instead. '
'This feature was deprecated after v3.7.0-5.0.pre.',
)
double? get dataRowHeight => dataRowMinHeight == dataRowMaxHeight ? dataRowMinHeight : null;
/// The minimum height of each row (excluding the row that contains column headings).
///
/// This value is optional and defaults to [kMinInteractiveDimension] if not
/// specified.
final double dataRowMinHeight;
/// The maximum height of each row (excluding the row that contains column headings).
///
/// This value is optional and defaults to kMinInteractiveDimension if not
/// specified.
final double dataRowMaxHeight;
/// The height of the heading row.
///
......@@ -518,7 +545,8 @@ class PaginatedDataTableState extends State<PaginatedDataTable> {
// Make sure no decoration is set on the DataTable
// from the theme, as its already wrapped in a Card.
decoration: const BoxDecoration(),
dataRowHeight: widget.dataRowHeight,
dataRowMinHeight: widget.dataRowMinHeight,
dataRowMaxHeight: widget.dataRowMaxHeight,
headingRowHeight: widget.headingRowHeight,
horizontalMargin: widget.horizontalMargin,
checkboxHorizontalMargin: widget.checkboxHorizontalMargin,
......
......@@ -629,14 +629,16 @@ void main() {
Widget buildCustomTable({
int? sortColumnIndex,
bool sortAscending = true,
double dataRowHeight = 48.0,
double? dataRowMinHeight,
double? dataRowMaxHeight,
double headingRowHeight = 56.0,
}) {
return DataTable(
sortColumnIndex: sortColumnIndex,
sortAscending: sortAscending,
onSelectAll: (bool? value) {},
dataRowHeight: dataRowHeight,
dataRowMinHeight: dataRowMinHeight,
dataRowMaxHeight: dataRowMaxHeight,
headingRowHeight: headingRowHeight,
columns: <DataColumn>[
const DataColumn(
......@@ -712,7 +714,7 @@ void main() {
Finder findFirstContainerFor(String text) => find.widgetWithText(Container, text).first;
expect(tester.getSize(findFirstContainerFor('Name')).height, 56.0);
expect(tester.getSize(findFirstContainerFor('Frozen yogurt')).height, 48.0);
expect(tester.getSize(findFirstContainerFor('Frozen yogurt')).height, kMinInteractiveDimension);
// CUSTOM VALUES
await tester.pumpWidget(MaterialApp(
......@@ -726,14 +728,105 @@ void main() {
expect(tester.getSize(findFirstContainerFor('Name')).height, 64.0);
await tester.pumpWidget(MaterialApp(
home: Material(child: buildCustomTable(dataRowHeight: 30.0)),
home: Material(child: buildCustomTable(dataRowMinHeight: 30.0, dataRowMaxHeight: 30.0)),
));
expect(tester.getSize(findFirstContainerFor('Frozen yogurt')).height, 30.0);
await tester.pumpWidget(MaterialApp(
home: Material(child: buildCustomTable(dataRowHeight: 56.0)),
home: Material(child: buildCustomTable(dataRowMinHeight: 0.0, dataRowMaxHeight: double.infinity)),
));
expect(tester.getSize(findFirstContainerFor('Frozen yogurt')).height, greaterThan(0.0));
});
testWidgets('DataTable custom row height one row taller than others', (WidgetTester tester) async {
const String multilineText = 'Line one.\nLine two.\nLine three.\nLine four.';
Widget buildCustomTable({
double? dataRowMinHeight,
double? dataRowMaxHeight,
}) {
return DataTable(
dataRowMinHeight: dataRowMinHeight,
dataRowMaxHeight: dataRowMaxHeight,
columns: const <DataColumn>[
DataColumn(
label: Text('SingleRowColumn'),
),
DataColumn(
label: Text('MultiRowColumn'),
),
],
rows: const <DataRow>[
DataRow(cells: <DataCell>[
DataCell(Text('Data')),
DataCell(Column(children: <Widget>[
Text(multilineText),
])),
]),
],
);
}
Finder findFirstContainerFor(String text) => find.widgetWithText(Container, text).first;
await tester.pumpWidget(MaterialApp(
home: Material(child: buildCustomTable(dataRowMinHeight: 0.0, dataRowMaxHeight: double.infinity)),
));
final double singleLineRowHeight = tester.getSize(findFirstContainerFor('Data')).height;
final double multilineRowHeight = tester.getSize(findFirstContainerFor(multilineText)).height;
expect(multilineRowHeight, greaterThan(singleLineRowHeight));
});
testWidgets('DataTable custom row height - separate test for deprecated dataRowHeight', (WidgetTester tester) async {
Widget buildCustomTable({
double dataRowHeight = 48.0,
}) {
return DataTable(
onSelectAll: (bool? value) {},
dataRowHeight: dataRowHeight,
columns: <DataColumn>[
const DataColumn(
label: Text('Name'),
tooltip: 'Name',
),
DataColumn(
label: const Text('Calories'),
tooltip: 'Calories',
numeric: true,
onSort: (int columnIndex, bool ascending) {},
),
],
rows: kDesserts.map<DataRow>((Dessert dessert) {
return DataRow(
key: ValueKey<String>(dessert.name),
onSelectChanged: (bool? selected) {},
cells: <DataCell>[
DataCell(
Text(dessert.name),
),
DataCell(
Text('${dessert.calories}'),
showEditIcon: true,
onTap: () {},
),
],
);
}).toList(),
);
}
// The finder matches with the Container of the cell content, as well as the
// Container wrapping the whole table. The first one is used to test row
// heights.
Finder findFirstContainerFor(String text) => find.widgetWithText(Container, text).first;
// CUSTOM VALUES
await tester.pumpWidget(MaterialApp(
home: Material(child: buildCustomTable(dataRowHeight: 30.0)),
));
expect(tester.getSize(findFirstContainerFor('Frozen yogurt')).height, 56.0);
expect(tester.getSize(findFirstContainerFor('Frozen yogurt')).height, 30.0);
});
testWidgets('DataTable custom horizontal padding - checkbox', (WidgetTester tester) async {
......@@ -1946,4 +2039,37 @@ void main() {
expect(material.clipBehavior, Clip.hardEdge);
expect(material.borderRadius, borderRadius);
});
testWidgets('DataTable dataRowMinHeight smaller or equal dataRowMaxHeight validation', (WidgetTester tester) async {
DataTable createDataTable() =>
DataTable(
columns: const <DataColumn>[DataColumn(label: Text('Column1'))],
rows: const <DataRow>[],
dataRowMinHeight: 2.0,
dataRowMaxHeight: 1.0,
);
expect(() => createDataTable(), throwsA(predicate((AssertionError e) =>
e.toString().contains('dataRowMaxHeight >= dataRowMinHeight'))));
});
testWidgets('DataTable dataRowHeight is not used together with dataRowMinHeight or dataRowMaxHeight', (WidgetTester tester) async {
DataTable createDataTable({double? dataRowHeight, double? dataRowMinHeight, double? dataRowMaxHeight}) =>
DataTable(
columns: const <DataColumn>[DataColumn(label: Text('Column1'))],
rows: const <DataRow>[],
dataRowHeight: dataRowHeight,
dataRowMinHeight: dataRowMinHeight,
dataRowMaxHeight: dataRowMaxHeight,
);
expect(() => createDataTable(dataRowHeight: 1.0, dataRowMinHeight: 2.0, dataRowMaxHeight: 2.0), throwsA(predicate((AssertionError e) =>
e.toString().contains('dataRowHeight == null || (dataRowMinHeight == null && dataRowMaxHeight == null)'))));
expect(() => createDataTable(dataRowHeight: 1.0, dataRowMaxHeight: 2.0), throwsA(predicate((AssertionError e) =>
e.toString().contains('dataRowHeight == null || (dataRowMinHeight == null && dataRowMaxHeight == null)'))));
expect(() => createDataTable(dataRowHeight: 1.0, dataRowMinHeight: 2.0), throwsA(predicate((AssertionError e) =>
e.toString().contains('dataRowHeight == null || (dataRowMinHeight == null && dataRowMaxHeight == null)'))));
});
}
......@@ -405,7 +405,9 @@ void main() {
final TestDataSource source = TestDataSource();
Widget buildCustomHeightPaginatedTable({
double dataRowHeight = 48.0,
double? dataRowHeight,
double? dataRowMinHeight,
double? dataRowMaxHeight,
double headingRowHeight = 56.0,
}) {
return PaginatedDataTable(
......@@ -423,6 +425,8 @@ void main() {
DataColumn(label: Text('Generation')),
],
dataRowHeight: dataRowHeight,
dataRowMinHeight: dataRowMinHeight,
dataRowMaxHeight: dataRowMaxHeight,
headingRowHeight: headingRowHeight,
);
}
......@@ -480,6 +484,13 @@ void main() {
expect(tester.renderObject<RenderBox>(
find.widgetWithText(Container, 'Frozen yogurt (0)').first,
).size.height, 56.0);
await tester.pumpWidget(MaterialApp(
home: Material(child: buildCustomHeightPaginatedTable(dataRowMinHeight: 51.0, dataRowMaxHeight: 51.0)),
));
expect(tester.renderObject<RenderBox>(
find.widgetWithText(Container, 'Frozen yogurt (0)').first,
).size.height, 51.0);
});
testWidgets('PaginatedDataTable custom horizontal padding - checkbox', (WidgetTester tester) async {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment