Skip to content

Migrate ListTile TextTheme TextStyle references to Material 3 #101900

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
Merged
HansMuller merged 1 commit into from
Apr 18, 2022
Merged

Migrate ListTile TextTheme TextStyle references to Material 3 #101900

HansMuller merged 1 commit into from
Apr 18, 2022

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Apr 14, 2022 •
edited by guidezpl
Loading

Updates ListTile [TextTheme] text style references to the Material 3 style names.

Fixes #102075

Part of: #91605 (fixes #89853)

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 14, 2022
@TahaTesser TahaTesser requested a review from HansMuller April 14, 2022 10:55
@HansMuller HansMuller mentioned this pull request Apr 14, 2022
Closed
70 tasks
@HansMuller HansMuller changed the title Migrate ListTile to Material 3 Migrate ListTile TextTheme TextStyle references to Material 3 Apr 14, 2022
HansMuller
HansMuller reviewed Apr 14, 2022
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

I've changed the title of this PR to narrow its scope. In a separate PR we could change the defaults for the ListTile.iconColor - when ThemeData.useMaterial3 = true - to match the Material 3 spec. That means that the default would be based on the theme's color scheme, not IconTheme. We're not trying to simplify making all of the icons in an app have the same color, but we don't want the defaults to be correct for each component type, per the spec.

HansMuller
HansMuller reviewed Apr 15, 2022
View reviewed changes
@TahaTesser TahaTesser force-pushed the M3_tile_icon_color_text branch from b9ba2c9 to 3c44cc7 Compare April 18, 2022 09:51
@TahaTesser TahaTesser force-pushed the M3_tile_icon_color_text branch from 3c44cc7 to 3866272 Compare April 18, 2022 09:56
@TahaTesser TahaTesser requested a review from HansMuller April 18, 2022 12:32
HansMuller
HansMuller approved these changes Apr 18, 2022
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

@HansMuller HansMuller merged commit 9f0bcfb into flutter:master Apr 18, 2022
@TahaTesser TahaTesser deleted the M3_tile_icon_color_text branch April 18, 2022 16:58
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 18, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 18, 2022
@engine-flutter-autoroll
9f0bcfb Update ListTile font (flutter/flutter#101900)
9df762c
@HansMuller
Copy link
Contributor

HansMuller commented Apr 19, 2022

This caused some internal test failures (Google internal link b/229707952). Most of them seem inconsequential, but there were a few cases where the a font became noticeably smaller. I'm going to revert this until we can track down the source of those failures (maybe a conditional useMaterial3 is needed somewhere).

@QuncCccccc

HansMuller pushed a commit that referenced this pull request Apr 19, 2022
Revert "Update ListTile font (#101900)"
1da4fd8 
This reverts commit 9f0bcfb.
@HansMuller HansMuller mentioned this pull request Apr 19, 2022
Merged
@TahaTesser
Copy link
Member Author

TahaTesser commented Apr 19, 2022

@HansMuller @QuncCccccc

I think this is caused by this line:

https://github.com/flutter/flutter/pull/101900/files#diff-c4ef7c798c820ecda6cab38be1c2d619d5349dc478889f3a54ebbe07f60c11f2L657

Earlier today I was looking into this new issue #102121 and I realized bodyText1 -> bodyLarge not bodyMedium. (which is in this PR)

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

Lines 262 to 266 in 078deb9

/// Used for emphasizing text that would otherwise be [bodyText2].
TextStyle? get bodyText1 => bodyLarge;
/// The default text style for [Material].
TextStyle? get bodyText2 => bodyMedium;

@TahaTesser
Copy link
Member Author

TahaTesser commented Apr 19, 2022 •
edited
Loading

After revert is completed, I can file reland PR to match the proper font, which is bodyLarge but I am also concerned about #102121, since it looks like spacing is incorrect in this newly added M3 font,

https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/typography.dart#L613-L614

@TahaTesser
Copy link
Member Author

TahaTesser commented Apr 19, 2022

(maybe a conditional useMaterial3 is needed somewhere).

This sounds great until we're sure M3 fonts are working as expected.

HansMuller added a commit that referenced this pull request Apr 19, 2022
@HansMuller
Revert "Update ListTile font (#101900)" (#102152)
ce6188b 
This reverts commit 9f0bcfb.
@TahaTesser TahaTesser mentioned this pull request Apr 19, 2022
Merged
8 tasks
@TahaTesser
Copy link
Member Author

TahaTesser commented Apr 19, 2022 •
edited
Loading

While relanding, I can see bodyLarge size is bigger than bodyText1 so reland is still using bodyMedium.

@TahaTesser TahaTesser mentioned this pull request Apr 19, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 21, 2022
@engine-flutter-autoroll
9f0bcfb Update ListTile font (flutter/flutter#101900)
10f2890
egramond pushed a commit to egramond/flutter that referenced this pull request May 5, 2022
@HansMuller @egramond
Revert "Update ListTile font (flutter#101900)" (flutter#102152)
9e494b0 
This reverts commit 9f0bcfb.
@Gabriel2048
Copy link

Gabriel2048 commented Mar 25, 2023

Was there a way to disable the default InkWell/ Splash behavior considered?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate ListTile TextTheme TextStyle references to Material 3 ☂️ Update TextTheme to use new Material 3 text styles

3 participants

@TahaTesser @HansMuller @Gabriel2048