Updated ListTile layout #17496

HansMuller · 2018-05-11T00:41:18Z

Updated ListTile's layout to match the current Material spec: https://material.io/design/components/lists.html#specs

Unless textScaleFactor causes the layout to overflow, the title and subtitle baselines are now located per the spec. Similarly the height of the tile and the layout of the title/subtitle relative to the leading widget are as specified.

Added a contentPadding parameter to ListTileTheme and ListTile to enable overriding the default 16dps padding on the tile's left and right .

gspencergoog · 2018-05-11T17:05:23Z

packages/flutter/lib/src/material/list_tile.dart

When I first read this, I was thinking that it specified the same padding for each of the items in the list tile separately, meaning that if you set it to 10.0, then there would be 20 pixels horizontally between the leading widget and the titles. It looks like this doesn't do that, but rather puts padding around the outside of the entire widget only, not the individual components (so setting it to 10.0 wouldn't affect internal horizontal padding). You might clear up the wording a bit.

I guess that's the confusing difference between the ListTiles' padding and the ListTile's padding.

I've changed it to:

/// The tile's internal padding. /// /// Insets a [ListTile]'s contents: its [leading], [title], [subtitle], /// and [trailing] widgets.

gspencergoog · 2018-05-11T17:05:48Z

packages/flutter/lib/src/material/list_tile.dart

Same comment as above to clarify what it affects.

gspencergoog · 2018-05-11T17:08:29Z

packages/flutter/lib/src/material/list_tile.dart

Wouldn't it be better to have the safe area outside of the padding? This way it might look a little close to a side notch if it's at the edge. And why only have it take effect horizontally?

The safe area has been within the padding and configured this way for about 6 months; since #13545

Maybe we should leave it alone?

@cbracken what do you think?

gspencergoog · 2018-05-11T17:15:15Z

packages/flutter/lib/src/material/list_tile.dart

Can you make these magic numbers into named constants?

gspencergoog · 2018-05-11T17:15:32Z

packages/flutter/lib/src/material/list_tile.dart

Ditto here, and below.

I haven't introduced one-time constants for the default tile heights and baselines because making up names for these values doesn't seems to make the code any more readable.

I did add a general comment about the dimensions and a pointer to the MD spec

Yay! We can add pointers to the MD spec again!

gspencergoog · 2018-05-11T17:40:59Z

packages/flutter/lib/src/material/list_tile.dart

Maybe convert this magic number into a named constant?

gspencergoog · 2018-05-11T17:47:40Z

packages/flutter/lib/src/material/list_tile.dart

Some of these (like this one) are only used once. It might be simpler to just have leading != null for that one conditional.

I had it that way at one point but I found it easier to read/change the code by defining hasFoo vars for all of the children

gspencergoog · 2018-05-11T22:41:32Z

packages/flutter/lib/src/material/list_tile.dart

    final Decoration decoration = new BoxDecoration(
-      border: new Border(
-        bottom: Divider.createBorderSide(context, color: color),
+     border: new Border(


Indentation is only one space: should be two.

gspencergoog · 2018-05-11T22:41:41Z

packages/flutter/lib/src/material/list_tile.dart

    while (iterator.moveNext()) {
      yield new DecoratedBox(
-        position: DecorationPosition.foreground,
+       position: DecorationPosition.foreground,


Indentation

gspencergoog · 2018-05-11T22:41:54Z

packages/flutter/lib/src/material/list_tile.dart

+       position: DecorationPosition.foreground,
        decoration: decoration,
-        child: tile,
+      child: tile,


Indentation

Thanks for catching these indentOs. I must be developing indentation blindness :-)

gspencergoog · 2018-05-11T22:44:45Z

packages/flutter/lib/src/material/list_tile.dart

+
+    final double titleStart = hasLeading ? leadingSize.width <= 40.0 ? 56.0 : leadingSize.width + 16.0 : 0.0;
+    final BoxConstraints textConstraints = looseConstraints.tighten(
+      width: tileWidth - titleStart - (hasTrailing ? trailingSize.width + 16.0 : 0.0),


Should that 16.0 also be _leadingTitleGap? (in which case it should probably be called _titleGap, or add a _trailingTitleGap).

Good point. I used _kTitleGap

gspencergoog · 2018-05-11T22:45:23Z

packages/flutter/lib/src/material/list_tile.dart

+      case TextDirection.rtl: {
+        if (hasLeading)
+          _positionBox(leading, new Offset(tileWidth - leadingSize.width, leadingY));
+        final double titleX = hasTrailing ? trailingSize.width + 16.0 : 0.0;


I think this 16.0 should also be a named constant (e.g. _titleGap) to keep it consistent with the one above.

gspencergoog · 2018-05-11T22:47:23Z

(modulo nits)

Hixie · 2018-05-14T23:17:26Z

packages/flutter/lib/src/material/list_tile.dart

+  static const double _kTitleGap = 16.0; // between the titles and the leading/trailing widgets
+
+  final Map<_ListTileSlot, RenderBox> slotToChild = <_ListTileSlot, RenderBox>{};
+  final Map<RenderBox, _ListTileSlot> childToSlot = <RenderBox, _ListTileSlot>{};


this is a pattern we've seen several times now, we might want to factor it out into a common foundation class

Hixie · 2018-05-14T23:18:05Z

packages/flutter/lib/src/material/list_tile.dart

+  }
+
+  @override
+  bool get sizedByParent => false;


isn't this the default?

Hixie · 2018-05-14T23:18:28Z

packages/flutter/lib/src/material/list_tile.dart

+
+    if (isOneLine)
+      return isDense ? 48.0 : 56.0;
+    else if (isTwoLine)


these elses can be omitted

Hixie · 2018-05-14T23:24:48Z

packages/flutter/lib/src/material/list_tile.dart

+  }
+}
+
+class _RenderListTileElement extends RenderObjectElement {


generally speaking, we put the element with the widget, and the renderobject after (or before) them.

This reverts commit ee019c0.

restored #17496 restored #17580

Hixie · 2018-11-27T07:18:30Z

packages/flutter/lib/src/material/list_tile.dart

+        tileHeight = titleSize.height + subtitleSize.height;
+        titleY = 0.0;
+        subtitleY = titleSize.height;
+      }


What are lines 780 to 795 based on?

googlebot added the cla: yes label May 11, 2018

gspencergoog reviewed May 11, 2018

View reviewed changes

HansMuller added 2 commits May 11, 2018 12:55

Updated ListTile layout

71ce1d1

updated per review feedback

ae94811

HansMuller force-pushed the updated_list_tile_layout branch from abfb2ae to ae94811 Compare May 11, 2018 19:55

Use SafeArea.minimum

3cf6071

gspencergoog approved these changes May 11, 2018

View reviewed changes

Updated per review feedback

3a4248c

HansMuller merged commit ee019c0 into flutter:master May 14, 2018

HansMuller deleted the updated_list_tile_layout branch May 14, 2018 15:23

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Updated ListTile layout (flutter#17496)

6feaebb

Hixie reviewed May 14, 2018

View reviewed changes

packages/flutter/lib/src/material/list_tile.dart

}

@override

bool get sizedByParent => false;

Copy link

Contributor

Hixie May 14, 2018

Choose a reason for hiding this comment

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

isn't this the default?

Hixie reviewed May 14, 2018

View reviewed changes

HansMuller pushed a commit that referenced this pull request May 15, 2018

Revert "Updated ListTile layout (#17496)"

6680c52

This reverts commit ee019c0.

This was referenced May 15, 2018

Revert "Updated ListTile layout" #17612

Merged

Change kRadialReactionRadius from 24 to 20 #17620

Merged

HansMuller added a commit to HansMuller/flutter that referenced this pull request May 16, 2018

restored flutter#17496

992dc76

HansMuller mentioned this pull request May 16, 2018

Restore the ListTile layout updates #17661

Merged

HansMuller pushed a commit that referenced this pull request May 16, 2018

Restore the ListTile layout updates (#17661)

60272ee

restored #17496 restored #17580

HansMuller mentioned this pull request May 23, 2018

Spacing between title and subtitle in non-dense ListTile doesn't match Material spec #17219

Closed

HansMuller mentioned this pull request Jul 24, 2018

ListTile title length is super short when trailing is a Row, in some version >0.3.1 #19657

Closed

Hixie reviewed Nov 27, 2018

View reviewed changes

This was referenced Mar 21, 2019

ListTile broken in 0.5.1 beta #20156

Closed

ListTile "leading" parameter stopped working #28765

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021

Updated ListTile layout #17496

Updated ListTile layout #17496

Uh oh!

Conversation

HansMuller commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HansMuller May 11, 2018 •

edited

Loading