Skip to content

Setting icon color to first ListTile in ExpansionTile. Fixes #23053 #23118

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 4 commits into from
Jan 2, 2019

Conversation

jmolins
Copy link
Contributor

@jmolins jmolins commented Oct 15, 2018

In issue #23053, developer reports being unable to set the trailing icon color in the first ListTile of the ExpansionTile.

This behaviour can be seen in the Gallery app. Go to Material > Expand/collapse list control. The trailing arrow does not get the same blue color the title gets.

The problem is the following:

The ExpansionTile sets an IconThemeData before creating the ListTile and sets the color to be used forward. This is the original code:

        children: <Widget>[
          IconTheme.merge(
            data: IconThemeData(color: _iconColor.value),
            child: ListTile(
              onTap: _handleTap,
              leading: widget.leading,
              title: DefaultTextStyle(
                style: Theme.of(context).textTheme.subhead.copyWith(color: titleColor),
                child: widget.title,
              ),
              trailing: widget.trailing ?? RotationTransition(
                turns: _iconTurns,
                child: const Icon(Icons.expand_more),
              ),
            ),
          ),
          ClipRect(
            child: Align(
              heightFactor: _heightFactor.value,
              child: child,
            ),
          ),
        ],

The problem with this code is that the ListTile itself creates another IconThemeData in its build() method. And this new IconThemeData does not take into account the one created outside in the ExpansionTile.

This PR substitutes the IconThemeData by a ListTileTheme that is used natively by the code inside the ListTile to set its style.

Since the ListTileTheme can also provide a text style, it is used to provide the style for the title so it is no longer necessary to create a DefaultTextStyle, that is also removed from the code.

@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2018

This looks pretty reasonable.

Can you add a test to make sure we don't accidentally revert this?

@jmolins
Copy link
Contributor Author

jmolins commented Nov 4, 2018

@Hixie, I have added the test as requested.

The test is based on the tests done for ListTile in

testWidgets('ListTileTheme', (WidgetTester tester) async {
.

In this case, though, since the ListTileTheme is just above the first ListTile, the tests apply only to this one. The test ensure that the theme in scope is correctly applied to the tile through the ListTileTheme.

@Hixie
Copy link
Contributor

Hixie commented Nov 15, 2018

LGTM

@Hixie
Copy link
Contributor

Hixie commented Nov 15, 2018

Thanks for your contribution! I'll land it when the bots go green.

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 28, 2018
@HansMuller
Copy link
Contributor

Landing this now (finally!).

@HansMuller HansMuller merged commit 4b87e33 into flutter:master Jan 2, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

5 participants