Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix DataCell overflows with column with multiple text widgets, row height doesn't adjust based on content height #70510 #102886

Closed
wants to merge 16 commits into from
Closed

Fix DataCell overflows with column with multiple text widgets, row height doesn't adjust based on content height #70510 #102886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
77c06b0
Fix DataCell overflows when cell height is large.
inouiw Apr 30, 2022
966aaf7
Add me to AUTHORS.
inouiw Apr 30, 2022
49cd421
Fix trailing spaces.
inouiw Apr 30, 2022
7c1fd39
Fix missing line.
inouiw Apr 30, 2022
d73de8c
Fix dartdoc error: unresolved doc reference
inouiw May 1, 2022
c4a86fd
Add class to store DataTable row height settings.
inouiw May 19, 2022
4661315
Merge branch 'master' of https://github.com/flutter/flutter into fix_…
inouiw May 19, 2022
ada3a90
Fix build warnings.
inouiw May 19, 2022
05bb9c7
Fix code analysis.
inouiw May 19, 2022
8adc783
Test and fix DataTableThemeData.copyWith
inouiw May 20, 2022
c98c020
Remove comments.
inouiw May 20, 2022
cd1f836
Prefer dataRowHeight over theme value.
inouiw May 20, 2022
a06aa53
Review changes
inouiw May 30, 2022
40a08ac
Review changes
inouiw May 30, 2022
89f858b
Review changes
inouiw May 30, 2022
7b9f809
Merge branch 'master' into fix_70510
inouiw Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,4 @@ Twin Sun, LLC <google-contrib@twinsunsolutions.com>
Taskulu LDA <contributions@taskulu.com>
Jonathan Joelson <jon@joelson.co>
Elsabe Ros <hello@elsabe.dev>
David Neuy <quantjump@gmail.com>
101 changes: 84 additions & 17 deletions packages/flutter/lib/src/material/data_table.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ class DataCell {
/// * [DataColumn], which describes a column in the data table.
/// * [DataRow], which contains the data for a row in the data table.
/// * [DataCell], which contains the data for a single cell in the data table.
/// * [DataRowHeightStyle], which contains height settings for data rows.
/// * [PaginatedDataTable], which shows part of the data in a data table and
/// provides controls for paging through the remainder of the data.
/// * <https://material.io/design/components/data-tables.html>
Expand Down Expand Up @@ -384,7 +385,12 @@ class DataTable extends StatelessWidget {
this.onSelectAll,
this.decoration,
this.dataRowColor,
this.dataRowHeight,
@Deprecated(
'Use dataRowHeightStyle instead. '
'This feature was deprecated after v3.1.0-0.0.pre.',
)
double? dataRowHeight,
DataRowHeightStyle? dataRowHeightStyle,
this.dataTextStyle,
this.headingRowColor,
this.headingRowHeight,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it reasonable to assume users would want the same for the header row height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The auto height for the data row is needed if you have some rows that are higher than other rows for example for multiline text. Then you do not want all rows to be e.g. twice as high but only the rows which need it.
For the header row, even though not as convenient as an auto height, it should be enough to be able to set a fixed height.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be enough to be able to set a fixed height.

How would that logic not apply to the general row height? :)

Expand All @@ -405,7 +411,8 @@ class DataTable extends StatelessWidget {
assert(rows != null),
assert(!rows.any((DataRow row) => row.cells.length != columns.length)),
assert(dividerThickness == null || dividerThickness >= 0),
_onlyTextColumn = _initOnlyTextColumn(columns);
_onlyTextColumn = _initOnlyTextColumn(columns),
dataRowHeightStyle = dataRowHeightStyle ?? (dataRowHeight != null ? DataRowHeightStyle.fixed(height: dataRowHeight) : null);

/// The configuration and labels for the columns in the table.
final List<DataColumn> columns;
Expand Down Expand Up @@ -489,14 +496,13 @@ class DataTable extends StatelessWidget {
/// {@endtemplate}
final MaterialStateProperty<Color?>? dataRowColor;

/// {@template flutter.material.dataTable.dataRowHeight}
/// The height of each row (excluding the row that contains column headings).
/// {@endtemplate}
/// {@template flutter.material.dataTable.dataRowHeightStyle}
/// The height settings of each row excluding the row that contains column headings.
///
/// If null, [DataTableThemeData.dataRowHeight] is used. This value defaults
/// to [kMinInteractiveDimension] to adhere to the Material Design
/// specifications.
final double? dataRowHeight;
/// If null, [DataTableThemeData.dataRowHeightStyle] is used.
/// Defaults to [DataRowHeightStyle.fixed] with the height sets to [kMinInteractiveDimension].
/// {@endtemplate}
final DataRowHeightStyle? dataRowHeightStyle;

/// {@template flutter.material.dataTable.dataTextStyle}
/// The text style for data rows.
Expand Down Expand Up @@ -795,6 +801,7 @@ class DataTable extends StatelessWidget {
Widget _buildDataCell({
required BuildContext context,
required EdgeInsetsGeometry padding,
required double? height,
required Widget label,
required bool numeric,
required bool placeholder,
Expand Down Expand Up @@ -823,13 +830,10 @@ class DataTable extends StatelessWidget {
?? dataTableTheme.dataTextStyle
?? themeData.dataTableTheme.dataTextStyle
?? themeData.textTheme.bodyText2!;
final double effectiveDataRowHeight = dataRowHeight
?? dataTableTheme.dataRowHeight
?? themeData.dataTableTheme.dataRowHeight
?? kMinInteractiveDimension;

label = Container(
padding: padding,
height: effectiveDataRowHeight,
height: height,
alignment: numeric ? Alignment.centerRight : AlignmentDirectional.centerStart,
child: DefaultTextStyle(
style: effectiveDataTextStyle.copyWith(
Expand Down Expand Up @@ -908,6 +912,13 @@ class DataTable extends StatelessWidget {
?? theme.dataTableTheme.columnSpacing
?? _columnSpacing;

final DataRowHeightStyle effectiveDataRowHeightStyle = dataRowHeightStyle
?? dataTableTheme.dataRowHeightStyle
?? theme.dataTableTheme.dataRowHeightStyle
?? const DataRowHeightStyle.fixed(height: kMinInteractiveDimension);
final double? effectiveDataRowHeight = effectiveDataRowHeightStyle.fixedHeight;
final double effectiveDataRowTopBottomPadding = effectiveDataRowHeightStyle.verticalPadding;

final List<TableColumnWidth> tableColumns = List<TableColumnWidth>.filled(columns.length + (displayCheckboxColumn ? 1 : 0), const _NullTableColumnWidth());
final List<TableRow> tableRows = List<TableRow>.generate(
rows.length + 1, // the +1 is for the header row
Expand Down Expand Up @@ -993,9 +1004,15 @@ class DataTable extends StatelessWidget {
paddingEnd = effectiveColumnSpacing / 2.0;
}

final EdgeInsetsDirectional padding = EdgeInsetsDirectional.only(
final EdgeInsetsDirectional headingPadding = EdgeInsetsDirectional.only(
start: paddingStart,
end: paddingEnd,
);
final EdgeInsetsDirectional cellPadding = EdgeInsetsDirectional.only(
start: paddingStart,
end: paddingEnd,
top: effectiveDataRowTopBottomPadding,
bottom: effectiveDataRowTopBottomPadding,
);
if (dataColumnIndex == _onlyTextColumn) {
tableColumns[displayColumnIndex] = const IntrinsicColumnWidth(flex: 1.0);
Expand All @@ -1004,7 +1021,7 @@ class DataTable extends StatelessWidget {
}
tableRows[0].children![displayColumnIndex] = _buildHeadingCell(
context: context,
padding: padding,
padding: headingPadding,
label: column.label,
tooltip: column.tooltip,
numeric: column.numeric,
Expand All @@ -1018,7 +1035,8 @@ class DataTable extends StatelessWidget {
final DataCell cell = row.cells[dataColumnIndex];
tableRows[rowIndex].children![displayColumnIndex] = _buildDataCell(
context: context,
padding: padding,
padding: cellPadding,
height: effectiveDataRowHeight,
label: cell.child,
numeric: column.numeric,
placeholder: cell.placeholder,
Expand All @@ -1043,6 +1061,7 @@ class DataTable extends StatelessWidget {
type: MaterialType.transparency,
child: Table(
columnWidths: tableColumns.asMap(),
defaultVerticalAlignment: TableCellVerticalAlignment.middle,
children: tableRows,
border: border,
),
Expand All @@ -1051,6 +1070,54 @@ class DataTable extends StatelessWidget {
}
}

/// Height settings for DataRows.
/// Either a fixed row height or a row height that adjusts to its content can be specified.
@immutable
class DataRowHeightStyle with Diagnosticable {
/// Creates height settings where all rows are created with the same fixed height.
const DataRowHeightStyle.fixed({
required double height
}) : fixedHeight = height, verticalPadding = 0.0;

/// Creates height settings where the row height adjusts to the content.
const DataRowHeightStyle.auto({
required this.verticalPadding,
Copy link
Contributor

@Piinks Piinks Jul 12, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit unnecessary. Could the user not just wrap the cell content in Padding? The framework tries to avoid enumerating padding as a property on widgets, deferring instead to the actual padding widget.
It seems like the padding is what makes the difference here. Is there not a way we can determine the row needs to be bigger without changing the user facing code? Like if the height is not set, and the child is larger than the default height?

Again, this is very expensive because you will need to determine the height of all of the children in the row to determine the ultimate height. I do not know that users will want the trade off in performance in exchange for an auto-size row.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit unnecessary. Could the user not just wrap the cell content in Padding?

Yes. I like the suggestion.

Is there not a way we can determine the row needs to be bigger without changing the user facing code?

The first commit used a different approach than the current code, it changed the meaning of the dataRowHeight parameter to minDataRowHeight and used it in BoxConstraints as constraints: BoxConstraints(minHeight: minDataRowHeight), (see 77c06b0)
The first commit also had an additional parameter topBottomRowPadding that seemed necessary because rows with one line appeared to have padding because the row height was much larger than the content but for rows with multiple lines the BoxConstraints(minHeight: minDataRowHeight) would apply and there was no padding. Without needing to support padding the original solution might be simplified.
So one solution would be to use the BoxConstraints approach again but without the additional topBottomRowPadding parameter. However, then I would suggest to rename dataRowHeight to minDataRowHeight because that is what it would represent. Using BoxConstraints instead of a fixed height would probably be more expensive to render but it can be tested if that really makes a difference.

One note about your concerns for changing user facing code. I suppose, for most users there is no depreciation warning because most users use the default height and do not explicitly set dataRowHeight. The users that have set dataRowHeight will get a depreciation warning. However I suppose most apps do not use a lot of DataTable widgets, so there are not many places that need to be adjusted.

}) : fixedHeight = null;

/// The height of each data row.
///
/// If set then all rows will get this fixed height, if null then content based height is used.
final double? fixedHeight;

/// The top and bottom padding of each data row.
///
/// If fixed height is used then this value is set to 0.0 because the content is vertically centered which makes padding unneeded.
inouiw marked this conversation as resolved.
Show resolved Hide resolved
inouiw marked this conversation as resolved.
Show resolved Hide resolved
final double verticalPadding;

@override
int get hashCode => Object.hash(fixedHeight, verticalPadding);

@override
bool operator ==(Object other) {
if (other.runtimeType != runtimeType) {
return false;
}
return other is DataRowHeightStyle
&& other.fixedHeight == fixedHeight
&& other.verticalPadding == verticalPadding;
}

@override
String toStringShort() => objectRuntimeType(this, 'DataRowHeightStyle');

@override
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
super.debugFillProperties(properties);
properties.add(DoubleProperty('fixedHeight', fixedHeight));
properties.add(DoubleProperty('verticalPadding', verticalPadding));
}
}

/// A rectangular area of a Material that responds to touch but clips
/// its ink splashes to the current table row of the nearest table.
///
Expand Down
46 changes: 37 additions & 9 deletions packages/flutter/lib/src/material/data_table_theme.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'dart:ui' show lerpDouble;
import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';

import 'data_table.dart';
import 'material_state.dart';
import 'theme.dart';

Expand Down Expand Up @@ -37,7 +38,12 @@ class DataTableThemeData with Diagnosticable {
const DataTableThemeData({
this.decoration,
this.dataRowColor,
this.dataRowHeight,
@Deprecated(
'Use dataRowHeightStyle instead. '
'This feature was deprecated after v3.1.0-0.0.pre.',
)
double? dataRowHeight,
DataRowHeightStyle? dataRowHeightStyle,
this.dataTextStyle,
this.headingRowColor,
this.headingRowHeight,
Expand All @@ -46,7 +52,9 @@ class DataTableThemeData with Diagnosticable {
this.columnSpacing,
this.dividerThickness,
this.checkboxHorizontalMargin,
});
}) :
_dataRowHeight = dataRowHeight,
_dataRowHeightStyle = dataRowHeightStyle;

/// {@macro flutter.material.dataTable.decoration}
final Decoration? decoration;
Expand All @@ -55,8 +63,12 @@ class DataTableThemeData with Diagnosticable {
/// {@macro flutter.material.DataTable.dataRowColor}
final MaterialStateProperty<Color?>? dataRowColor;

/// {@macro flutter.material.dataTable.dataRowHeight}
final double? dataRowHeight;
final double? _dataRowHeight;
final DataRowHeightStyle? _dataRowHeightStyle;

/// {@macro flutter.material.dataTable.dataRowHeightStyle}
DataRowHeightStyle? get dataRowHeightStyle
=> _dataRowHeightStyle ?? (_dataRowHeight != null ? DataRowHeightStyle.fixed(height: _dataRowHeight!) : null);

/// {@macro flutter.material.dataTable.dataTextStyle}
final TextStyle? dataTextStyle;
Expand Down Expand Up @@ -88,7 +100,12 @@ class DataTableThemeData with Diagnosticable {
DataTableThemeData copyWith({
Decoration? decoration,
MaterialStateProperty<Color?>? dataRowColor,
@Deprecated(
'Use dataRowHeightStyle instead. '
'This feature was deprecated after v3.1.0-0.0.pre.',
)
double? dataRowHeight,
DataRowHeightStyle? dataRowHeightStyle,
TextStyle? dataTextStyle,
MaterialStateProperty<Color?>? headingRowColor,
double? headingRowHeight,
Expand All @@ -101,7 +118,7 @@ class DataTableThemeData with Diagnosticable {
return DataTableThemeData(
decoration: decoration ?? this.decoration,
dataRowColor: dataRowColor ?? this.dataRowColor,
dataRowHeight: dataRowHeight ?? this.dataRowHeight,
dataRowHeightStyle: dataRowHeightStyle ?? (dataRowHeight != null ? DataRowHeightStyle.fixed(height: dataRowHeight) : null) ?? this.dataRowHeightStyle,
dataTextStyle: dataTextStyle ?? this.dataTextStyle,
headingRowColor: headingRowColor ?? this.headingRowColor,
headingRowHeight: headingRowHeight ?? this.headingRowHeight,
Expand All @@ -123,7 +140,7 @@ 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),
dataRowHeightStyle: _lerpDataRowHeightStyle(a.dataRowHeightStyle, b.dataRowHeightStyle, 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),
Expand All @@ -139,7 +156,7 @@ class DataTableThemeData with Diagnosticable {
int get hashCode => Object.hash(
decoration,
dataRowColor,
dataRowHeight,
dataRowHeightStyle,
dataTextStyle,
headingRowColor,
headingRowHeight,
Expand All @@ -161,7 +178,7 @@ class DataTableThemeData with Diagnosticable {
return other is DataTableThemeData
&& other.decoration == decoration
&& other.dataRowColor == dataRowColor
&& other.dataRowHeight == dataRowHeight
&& other.dataRowHeightStyle == dataRowHeightStyle
&& other.dataTextStyle == dataTextStyle
&& other.headingRowColor == headingRowColor
&& other.headingRowHeight == headingRowHeight
Expand All @@ -177,7 +194,7 @@ 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(DiagnosticsProperty<DataRowHeightStyle>('dataRowHeightStyle', dataRowHeightStyle, 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));
Expand All @@ -187,6 +204,17 @@ class DataTableThemeData with Diagnosticable {
properties.add(DoubleProperty('dividerThickness', dividerThickness, defaultValue: null));
properties.add(DoubleProperty('checkboxHorizontalMargin', checkboxHorizontalMargin, defaultValue: null));
}

/// Interpolate between two [DataRowHeightStyle] instances.
static DataRowHeightStyle? _lerpDataRowHeightStyle(DataRowHeightStyle? a, DataRowHeightStyle? b, double t) {
if (a == null && b == null) {
return null;
}
if (a?.fixedHeight != null || b?.fixedHeight != null) {
return DataRowHeightStyle.fixed(height: lerpDouble(a?.fixedHeight, b?.fixedHeight, t) ?? 0.0);
}
return DataRowHeightStyle.auto(verticalPadding: lerpDouble(a?.verticalPadding, b?.verticalPadding, t) ?? 0.0);
}
}

/// Applies a data table theme to descendant [DataTable] widgets.
Expand Down
21 changes: 11 additions & 10 deletions packages/flutter/lib/src/material/paginated_data_table.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import 'package:flutter/gestures.dart' show DragStartBehavior;
import 'package:flutter/widgets.dart';

import 'card.dart';
import 'constants.dart';
import 'data_table.dart';
import 'data_table_source.dart';
import 'debug.dart';
Expand Down Expand Up @@ -71,7 +70,12 @@ class PaginatedDataTable extends StatefulWidget {
this.sortColumnIndex,
this.sortAscending = true,
this.onSelectAll,
this.dataRowHeight = kMinInteractiveDimension,
@Deprecated(
'Use dataRowHeightStyle instead. '
'This feature was deprecated after v3.1.0-0.0.pre.',
)
final double? dataRowHeight,
DataRowHeightStyle? dataRowHeightStyle,
this.headingRowHeight = 56.0,
this.horizontalMargin = 24.0,
this.columnSpacing = 56.0,
Expand All @@ -94,7 +98,6 @@ class PaginatedDataTable extends StatefulWidget {
assert(columns.isNotEmpty),
assert(sortColumnIndex == null || (sortColumnIndex >= 0 && sortColumnIndex < columns.length)),
assert(sortAscending != null),
assert(dataRowHeight != null),
assert(headingRowHeight != null),
assert(horizontalMargin != null),
assert(columnSpacing != null),
Expand All @@ -112,7 +115,8 @@ class PaginatedDataTable extends StatefulWidget {
assert(!(controller != null && (primary ?? false)),
'Primary ScrollViews obtain their ScrollController via inheritance from a PrimaryScrollController widget. '
'You cannot both set primary to true and pass an explicit controller.',
);
),
dataRowHeightStyle = dataRowHeightStyle ?? (dataRowHeight != null ? DataRowHeightStyle.fixed(height: dataRowHeight) : null);

/// The table card's optional header.
///
Expand Down Expand Up @@ -154,11 +158,8 @@ class PaginatedDataTable extends StatefulWidget {
/// See [DataTable.onSelectAll].
final ValueSetter<bool?>? onSelectAll;

/// The height of each row (excluding the row that contains column headings).
///
/// This value is optional and defaults to kMinInteractiveDimension if not
/// specified.
final double dataRowHeight;
/// {@macro flutter.material.dataTable.dataRowHeightStyle}
final DataRowHeightStyle? dataRowHeightStyle;

/// The height of the heading row.
///
Expand Down Expand Up @@ -529,7 +530,7 @@ 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,
dataRowHeightStyle: widget.dataRowHeightStyle,
headingRowHeight: widget.headingRowHeight,
horizontalMargin: widget.horizontalMargin,
checkboxHorizontalMargin: widget.checkboxHorizontalMargin,
Expand Down