-
Notifications
You must be signed in to change notification settings - Fork 29.4k
[Table] adds rowSpacing property #63322
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of reminds me of #16957, which was decided not to be added to the framework.
I could see how supporting row spacing would logically call on support for Column spacing as well.
If we follow the recommendation in the Row/Column spacing discussion in #16957, maybe adding an empty TableRow in between the other TableRows is a better approach? Not sure.
@Piinks Unlike
There may be other ways I don't know, And I think we should compare the In addition, the For reference, in the widget of the week introduces the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also not convinced that adding this is a good idea. If we'd do this, we'd probably also have to provide a way to add padding between the cells in one row. Just having rowPadding seems odd. It's also not terribly difficult to get this spacing without extra framework support:
TableRow spacer = TableRow(
children: <Widget>[
SizedBox(height: 10),
SizedBox(height: 10),
SizedBox(height: 10),
],
);
Table(
children: [
TableRow(
children: <Widget>[
Container(child: Text('1'), height: 100),
Container(child: Text('2'), height: 100),
Container(child: Text('3'), height: 100),
],
),
spacer,
TableRow(
children: <Widget>[
Container(child: Text('4'), height: 100),
Container(child: Text('5'), height: 100),
Container(child: Text('6'), height: 100),
],
),
spacer,
TableRow(
children: <Widget>[
Container(child: Text('7'), height: 100),
Container(child: Text('8'), height: 100),
Container(child: Text('9'), height: 100),
],
),
],
),
// then, lay out each row | ||
double rowTop = 0.0; | ||
for (int y = 0; y < rows; y += 1) { | ||
if (y != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably, this also needs changes to the intrinsics methods.
If adding rowSpacing isn't a good idea, it might be better to close this PR and the related issue. 🙏 |
Description
Adds a
rowSpacing
property to theTable
class.Related Issues
Fixes #52248
Tests
I added the following tests:
TableRow
.Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.