Skip to content
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

Adds displayFontFamily and bodyFontFamily to TextTheme apply. #148603

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adds displayFontFamily and bodyFontFamily to TextTheme apply. #148603

wants to merge 1 commit into from
+72 −30

Conversation

chinmoy12c
Copy link
Member

This PR adds displayFontFamily and bodyFontFamily to TextTheme.apply() method.

Fixes: #148510

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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 18, 2024
@Piinks Piinks self-requested a review May 22, 2024 18:22
Piinks
Piinks reviewed May 30, 2024
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

I am not sure this is a change we should make. The new display and body properties here don't seem to relate to all the text styles they are applied to.
If we add individual font families for display and body, does that mean we might also have to add them for headline, title, and label? Or what is folks want to specify a font family for each variant of small, medium and large?

I don't know that we want to enumerate every option on this method, since at that point folks would probably be better configuring their TextTheme another way.

packages/flutter/lib/src/material/text_theme.dart
@@ -355,12 +355,20 @@ class TextTheme with Diagnosticable {
/// [displaySmall], [headlineLarge], [headlineMedium], and [bodySmall]. The
/// `bodyColor` is applied to the remaining text styles.
///
/// The `displayFontFamily` is applied to [displayLarge], [displayMedium],
/// [displaySmall], [headlineLarge], [headlineMedium], and [bodySmall]. The
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would display apply to bodySmall?

Copy link
Contributor

Choose a reason for hiding this comment

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

And how does it relate to the headline ones as well?

packages/flutter/lib/src/material/text_theme.dart
@@ -355,12 +355,20 @@ class TextTheme with Diagnosticable {
/// [displaySmall], [headlineLarge], [headlineMedium], and [bodySmall]. The
/// `bodyColor` is applied to the remaining text styles.
///
/// The `displayFontFamily` is applied to [displayLarge], [displayMedium],
/// [displaySmall], [headlineLarge], [headlineMedium], and [bodySmall]. The
/// `bodyFontFamily` is applied to the remaining text styles.
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the remaining style? And how do these relate to fontFamily and fontFamilyFallback?

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

@Piinks Piinks Piinks left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
a: text input Entering text in a text field or keyboard related problems 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.

Add bodyFontFamily and displayFontFamily to TextTheme.apply()
2 participants
@chinmoy12c @Piinks