Platform-specific typography #6634

cbracken · 2016-11-01T23:14:06Z

No description provided.

cbracken · 2016-11-01T23:16:45Z

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

+  const Typography._(this.black, this.white);
+
+  /// Returns the default typography for the specified platform.
+  static Typography forPlatform(TargetPlatform platform) {


Thoughts on static forPlatform vs a factory ctor with required TargetPlatform? The only precedent I could find for a sort-of similar platform-specific ctor was the scroll behaviour delegate, but that's a type that actually gets used as a delegate as opposed to this case, which is just a util lookup.

Another option would be to just add black/white getters that take a platform.

Not sure I understand what you're asking.

Basically, anyone have a preference for factory Typography({@required TargetPlatform platform}) vs the code in this PR?

Ah, yes. I think I prefer the constructor.

Also, constructor before fields please.

(the rule is "code should be in the order it's run", to make it easier to read. The constructors are run before you do anything with fields, hence they go first.)

cbracken · 2016-11-02T00:47:50Z

Rebased this on top of #6640 to get the tests happy.

Hixie · 2016-11-02T00:54:19Z

dev/manual_tests/card_collection.dart

@@ -358,6 +356,9 @@ class CardCollectionState extends State<CardCollection> {
    if (_dismissDirection == DismissDirection.endToStart)
      rightArrowIcon = new Opacity(opacity: 0.1, child: rightArrowIcon);

+    ThemeData theme = Theme.of(context);


these can be final

Hixie · 2016-11-02T00:54:39Z

dev/manual_tests/raw_keyboard.dart

@@ -41,6 +41,7 @@ class _HardwareKeyDemoState extends State<RawKeyboardDemo> {

  @override
  Widget build(BuildContext context)  {
+    TextTheme textTheme = Theme.of(context).textTheme;


these can be final

Hixie · 2016-11-02T00:55:40Z

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

@@ -77,6 +78,32 @@ class TextTheme {
      caption  = const TextStyle(fontFamily: 'Roboto', inherit: false, fontSize:  12.0, fontWeight: FontWeight.w400, color: Colors.white70, textBaseline: TextBaseline.alphabetic),
      button   = const TextStyle(fontFamily: 'Roboto', inherit: false, fontSize:  14.0, fontWeight: FontWeight.w500, color: Colors.white,   textBaseline: TextBaseline.alphabetic);



rename _black and _white as well.

Maybe blackMountainView and blackCupertino etc.

I like it. Done.

cbracken · 2016-11-02T16:59:10Z

Comments addressed and re-squashed. PTAL.

Hixie · 2016-11-02T17:10:04Z

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

+  const Typography._(this.black, this.white);
+
+  /// Creates the default typography for the specified platform.
+  factory Typography({@required TargetPlatform platform}) {


style nit: spaces around the arguments please.

Hixie · 2016-11-02T17:10:32Z

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

+    switch (platform) {
+      case TargetPlatform.android:
+      case TargetPlatform.fuchsia:
+        typography = const Typography._(


you can just return here rather than setting a variable then returning it.
that also lets you remove the break.

Hixie · 2016-11-02T17:10:41Z

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

+        break;
+    }
+    assert(typography != null);
+    return typography;


then you'd just return null here.

Hixie · 2016-11-02T17:10:48Z

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

+
+  /// Creates the default typography for the specified platform.
+  factory Typography({@required TargetPlatform platform}) {
+    Typography typography;


assert platform != null

Hixie · 2016-11-02T17:11:49Z

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

-/// material design. The black text theme, which uses dark glyphs, is used on
-/// light backgrounds in light themes. The white text theme, which uses light
-/// glyphs, is used in dark themes and on dark backgrounds in in light themes.
+/// [black] and [white] define the two text themes used in material design. The


sentences should start with a capital letter. To make that work here, you can write it as something like The [black] and [white] properties provide...

Hixie · 2016-11-02T17:12:16Z

Ideally this would have some sort of test.

LGTM.

cbracken · 2016-11-02T18:47:16Z

Added a minimal test to ensure that typography is defined for all target platforms. Not a ton more we can test without getting into testing constants territory. Maybe just a test that iOS does its own thing?

cbracken · 2016-11-02T21:32:54Z

Added tests that ThemeData defaults to platform-specific font family and that ensure that we select the appropriate font family according to Apple's typography guidelines.

On Android and Fuchsia, default to Roboto. On iOS, use San Francisco.

cbracken · 2016-11-02T22:41:04Z

Fixes #4956

cbracken commented Nov 1, 2016

View reviewed changes

cbracken force-pushed the platform-specific-typography branch from 7dd9d08 to 41c4b98 Compare November 2, 2016 00:45

Hixie reviewed Nov 2, 2016

View reviewed changes

cbracken force-pushed the platform-specific-typography branch 5 times, most recently from 23cd372 to 9685ce7 Compare November 2, 2016 16:57

Hixie reviewed Nov 2, 2016

View reviewed changes

cbracken force-pushed the platform-specific-typography branch 2 times, most recently from 0b6c035 to 227aa6d Compare November 2, 2016 18:12

cbracken force-pushed the platform-specific-typography branch 2 times, most recently from 074300d to 43d7c33 Compare November 2, 2016 21:31

cbracken force-pushed the platform-specific-typography branch from 43d7c33 to b805a6a Compare November 2, 2016 21:51

Support per-platform default typography

4875cda

On Android and Fuchsia, default to Roboto. On iOS, use San Francisco.

cbracken force-pushed the platform-specific-typography branch from b805a6a to 4875cda Compare November 2, 2016 22:23

cbracken changed the title ~~Platform specific typography~~ Platform-specific typography Nov 2, 2016

cbracken merged commit abcfc42 into flutter:master Nov 2, 2016

cbracken mentioned this pull request Nov 2, 2016

Default to San Francisco font for iOS 9 #4956

Closed

cbracken deleted the platform-specific-typography branch January 24, 2017 03:52

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

		@@ -77,6 +78,32 @@ class TextTheme {
		caption = const TextStyle(fontFamily: 'Roboto', inherit: false, fontSize: 12.0, fontWeight: FontWeight.w400, color: Colors.white70, textBaseline: TextBaseline.alphabetic),
		button = const TextStyle(fontFamily: 'Roboto', inherit: false, fontSize: 14.0, fontWeight: FontWeight.w500, color: Colors.white, textBaseline: TextBaseline.alphabetic);

Platform-specific typography #6634

Platform-specific typography #6634

Uh oh!

Conversation

cbracken commented Nov 1, 2016

Uh oh!

cbracken Nov 1, 2016 • 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!

cbracken Nov 2, 2016 • 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!

cbracken commented Nov 2, 2016

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!

cbracken commented Nov 2, 2016

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!

Hixie commented Nov 2, 2016

Uh oh!

cbracken commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken commented Nov 2, 2016

Uh oh!

cbracken commented Nov 2, 2016

Uh oh!

Uh oh!

cbracken Nov 1, 2016 •

edited

Loading

cbracken Nov 2, 2016 •

edited

Loading

cbracken commented Nov 2, 2016 •

edited

Loading