-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Adds displayFontFamily
and bodyFontFamily
to TextTheme apply.
#148603
base: master
Are you sure you want to change the base?
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.
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.
@@ -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 |
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.
Why would display apply to bodySmall?
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.
And how does it relate to the headline ones as well?
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.
Hey this was done basis what displayColor
follows. Based on the reply I received here: https://discord.com/channels/608014603317936148/608021234516754444/1242440562452332565 I assume this is not relevant now?
@@ -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. |
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.
What are the remaining style? And how do these relate to fontFamily and fontFamilyFallback?
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.
The other styles include
headlineSmall
titleLarge
titleMedium
titleSmall
bodyLarge
bodyMedium
bodySmall
labelLarge
labelMedium
labelSmall
This is the config that displayColor
follows.
@chinmoy12c greetings from triage, would you like to continue working on this change? |
Yes, sorry for the delay. Just a bit crunched on time rn 😅 |
This PR adds
displayFontFamily
andbodyFontFamily
toTextTheme.apply()
method.Fixes: #148510
Pre-launch Checklist
///
).