Skip to content

Fix a DataTable crash and improve some docs #100959

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.

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

Merged
merged 5 commits into from
Apr 19, 2022

Conversation

xu-baolin
Copy link
Member

Fixes #100952
Fixes #100633

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 29, 2022
@xu-baolin xu-baolin requested a review from dkwingsmt March 29, 2022 11:55
/// by default, this widget will only occupy minimal space. It is
/// recommended to wrap this with an [Expanded] or [Flexible] to control how
/// it flexes. Otherwise, an exception will occur when the the available space
/// is insufficient.
Copy link
Member Author

Choose a reason for hiding this comment

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

fixes #100633

@xu-baolin
Copy link
Member Author

@dkwingsmt Hi, would you like take a look at this patch : )

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Apr 19, 2022

(I'm sorry for failing to notice this!)
Ok the code looks good, and the doc change is awesome. But I'm wondering, does it feel the developer will always want to wrap it with an Expanded? Should we just make it extended by default?

Comment on lines 50 to 51
/// This widget and the sort indicator (if visible) will wrap with a [Row],
/// by default, this widget will only occupy minimal space. It is
Copy link
Contributor

@dkwingsmt dkwingsmt Apr 19, 2022

Choose a reason for hiding this comment

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

The first sentence doesn't make much sense. Depending on what you mean, maybe

The [label] is placed within a [Row] along with the sort indicator (if applicable). By default, [label] only occupy minimal space.

/// you can wrap it with an [Expanded].
/// This widget and the sort indicator (if visible) will wrap with a [Row],
/// by default, this widget will only occupy minimal space. It is
/// recommended to wrap this with an [Expanded] or [Flexible] to control how
Copy link
Contributor

@dkwingsmt dkwingsmt Apr 19, 2022

Choose a reason for hiding this comment

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

It is recommended to place the label content in an [Expanded] or [Flexible] as [label] to control how the content flexes.

@@ -1050,26 +1051,26 @@ class RenderTable extends RenderBox {
if (rows * columns == 0) {
// TODO(ianh): if columns is zero, this should be zero width
// TODO(ianh): if columns is not zero, this should be based on the column width specifications
_tableWidth = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this resolve the TODOs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not quite understood what the todos mean, but I think maybe they can be removed.

@xu-baolin
Copy link
Member Author

(I'm sorry for failing to notice this!) Ok the code looks good, and the doc change is awesome. But I'm wondering, does it feel the developer will always want to wrap it with an Expanded? Should we just make it extended by default?

As you have said here, this may break something.
In addition, folks may already wrap it under Expanded, and redundant Expanded will throw.

/// sort indicator (if applicable). By default, [label] only occupy minimal
/// space. It is recommended to place the label content in an [Expanded] or
/// [Flexible] as [label] to control how the content flexes. Otherwise,
/// an exception will occur when the the available space is insufficient.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// an exception will occur when the the available space is insufficient.
/// an exception will occur when the available space is insufficient.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM except for nit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataTable with borders crashes when paint in a narrow space Flutter DataTable sample overflows if column heading is made a little longer
3 participants