Skip to content

Commit 03d16b8

Browse files
chrisbobbechrisbobbe
committed
ui: Set letter spacing to 1% in buttons
It's a bit annoying that we have to repeat all of elevatedButtonTheme filledButtonTheme menuButtonTheme outlinedButtonTheme segmentedButtonTheme textButtonTheme but anyway, here we are; I don't think I've left any out. [ThemeData.buttonTheme] *sounds* like it might be one central place to define button text's letter spacing...but on a closer look, it doesn't allow setting letter spacing, and moreover it's deprecated, with these various FooButtonThemes meant to replace it. Fixes: #548
1 parent dd97594 commit 03d16b8

File tree

4 files changed

+155
-1
lines changed

4 files changed

+155
-1
lines changed

‎lib/widgets/app.dart

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,8 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
128128
(_) => widget._declareReady());
129129
}
130130
GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
131-
return child!;
131+
return Theme(data: innerThemeData(context),
132+
child: child!);
132133
},
133134

134135
// We use onGenerateInitialRoutes for the real work of specifying the

‎lib/widgets/theme.dart

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,42 @@ ThemeData outerThemeData(BuildContext context) {
3737
/// This is chosen as the sRGB midpoint of the Zulip logo's gradient.
3838
// As computed by Anders: https://github.com/zulip/zulip-mobile/pull/4467
3939
const kZulipBrandColor = Color.fromRGBO(0x64, 0x92, 0xfe, 1);
40+
41+
/// A [ThemeData] that depends on [outerThemeData] and copies it with additions.
42+
///
43+
/// [context] must be below a [Theme] that provides [outerThemeData],
44+
/// so that this can use [Theme.of] to synthesize a [ThemeData.textTheme]
45+
/// based on [outerThemeData]'s [ThemeData.typography].
46+
// TODO(upstream) nicer if this could return a [ThemeData] with just the added
47+
// fields, instead of having to call `copyWith` on `Theme.of(context)`.
48+
// Then callers could call a hypothetical ThemeData.merge method with it.
49+
// But such a method doesn't exist yet:
50+
// https://github.com/flutter/flutter/issues/43823
51+
ThemeData innerThemeData(BuildContext context) {
52+
final theme = Theme.of(context);
53+
assert(theme.textTheme.labelLarge!.debugLabel!.contains('zulipTypography'));
54+
return theme.copyWith(
55+
elevatedButtonTheme: ElevatedButtonThemeData( style: _commonButtonStyle(context)),
56+
filledButtonTheme: FilledButtonThemeData( style: _commonButtonStyle(context)),
57+
iconButtonTheme: IconButtonThemeData( style: _commonButtonStyle(context)),
58+
menuButtonTheme: MenuButtonThemeData( style: _commonButtonStyle(context)),
59+
outlinedButtonTheme: OutlinedButtonThemeData( style: _commonButtonStyle(context)),
60+
segmentedButtonTheme: SegmentedButtonThemeData(style: _commonButtonStyle(context)),
61+
textButtonTheme: TextButtonThemeData( style: _commonButtonStyle(context)),
62+
);
63+
}
64+
65+
ButtonStyle _commonButtonStyle(BuildContext context) {
66+
// labelLarge is the default for all kinds of buttons that can have text. See:
67+
// [ElevatedButton.defaultStyleOf], [FilledButton.defaultStyleOf],
68+
// [MenuItemButton.defaultStyleOf], [MenuItemButton.defaultStyleOf],
69+
// [OutlinedButton.defaultStyleOf], `defaults` in [SegmentedButton.build],
70+
// and [TextButton.defaultStyleOf].
71+
final fontSize = Theme.of(context).textTheme.labelLarge!.fontSize!;
72+
return ButtonStyle(
73+
textStyle: WidgetStatePropertyAll(TextStyle(
74+
// If changing `fontSize` or `letterSpacing`, change the other too.
75+
fontSize: fontSize,
76+
letterSpacing: proportionalLetterSpacing(context,
77+
0.01, baseFontSize: fontSize))));
78+
}

‎test/flutter_checks.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ extension ValueNotifierChecks<T> on Subject<ValueNotifier<T>> {
5757

5858
extension TextChecks on Subject<Text> {
5959
Subject<String?> get data => has((t) => t.data, 'data');
60+
Subject<TextStyle?> get style => has((t) => t.style, 'style');
6061
}
6162

6263
extension TextFieldChecks on Subject<TextField> {
@@ -65,6 +66,7 @@ extension TextFieldChecks on Subject<TextField> {
6566

6667
extension TextStyleChecks on Subject<TextStyle> {
6768
Subject<bool> get inherit => has((t) => t.inherit, 'inherit');
69+
Subject<double?> get fontSize => has((t) => t.fontSize, 'fontSize');
6870
Subject<FontWeight?> get fontWeight => has((t) => t.fontWeight, 'fontWeight');
6971
Subject<double?> get letterSpacing => has((t) => t.letterSpacing, 'letterSpacing');
7072
Subject<List<FontVariation>?> get fontVariations => has((t) => t.fontVariations, 'fontVariations');

‎test/widgets/theme_test.dart

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
import 'package:checks/checks.dart';
2+
import 'package:flutter/material.dart';
3+
import 'package:flutter/rendering.dart';
4+
import 'package:flutter_test/flutter_test.dart';
5+
import 'package:zulip/widgets/text.dart';
6+
import 'package:zulip/widgets/theme.dart';
7+
8+
import '../flutter_checks.dart';
9+
10+
void main() {
11+
group('innerThemeData', () {
12+
testWidgets('smoke', (tester) async {
13+
ThemeData? outerThemeDataValue;
14+
ThemeData? value;
15+
await tester.pumpWidget(
16+
Builder(builder: (context) => MaterialApp(
17+
theme: outerThemeData(context),
18+
home: Builder(builder: (context) {
19+
outerThemeDataValue = Theme.of(context);
20+
return Theme(
21+
data: innerThemeData(context),
22+
child: Builder(builder: (context) {
23+
value = Theme.of(context);
24+
return const Placeholder();
25+
}));
26+
}))));
27+
28+
final outerElevatedButtonLetterSpacing = outerThemeDataValue
29+
!.elevatedButtonTheme.style?.textStyle?.resolve({})?.letterSpacing;
30+
final elevatedButtonLetterSpacing = value
31+
!.elevatedButtonTheme.style?.textStyle?.resolve({})?.letterSpacing;
32+
check(outerElevatedButtonLetterSpacing).isNull();
33+
check(elevatedButtonLetterSpacing).isNotNull();
34+
35+
// innerThemeData should extend outerThemeData using [ThemeData.copyWith]
36+
// (at least for now, lacking a ThemeData.merge method). Pick a value that
37+
// we set in outerThemeData and check that it hasn't been dropped.
38+
check(value!.scaffoldBackgroundColor)
39+
..equals(outerThemeDataValue!.scaffoldBackgroundColor)
40+
..equals(const Color(0xfff6f6f6));
41+
});
42+
43+
group('button text size and letter spacing', () {
44+
Future<void> testButtonLetterSpacing(
45+
String description, {
46+
required Widget Function(BuildContext context, String text) buttonBuilder,
47+
double? ambientTextScaleFactor,
48+
}) async {
49+
testWidgets(description, (WidgetTester tester) async {
50+
if (ambientTextScaleFactor != null) {
51+
tester.platformDispatcher.textScaleFactorTestValue = ambientTextScaleFactor;
52+
addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);
53+
}
54+
const buttonText = 'Zulip';
55+
double? expectedFontSize;
56+
double? expectedLetterSpacing;
57+
await tester.pumpWidget(
58+
Builder(builder: (context) => MaterialApp(
59+
theme: outerThemeData(context),
60+
home: Builder(builder: (context) {
61+
expectedFontSize = Theme.of(context).textTheme.labelLarge!.fontSize!;
62+
expectedLetterSpacing = proportionalLetterSpacing(context,
63+
0.01, baseFontSize: expectedFontSize!);
64+
return Theme(data: innerThemeData(context),
65+
child: Builder(builder: (context) =>
66+
buttonBuilder(context, buttonText)));
67+
}))));
68+
69+
final text = tester.renderObject<RenderParagraph>(find.text(buttonText)).text;
70+
check(text.style!)
71+
..fontSize.equals(expectedFontSize)
72+
..letterSpacing.equals(expectedLetterSpacing);
73+
});
74+
}
75+
76+
testButtonLetterSpacing('with device text size adjusted',
77+
ambientTextScaleFactor: 2.0,
78+
buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},
79+
child: Text(text)));
80+
81+
testButtonLetterSpacing('ElevatedButton',
82+
buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},
83+
child: Text(text)));
84+
85+
testButtonLetterSpacing('FilledButton',
86+
buttonBuilder: (context, text) => FilledButton(onPressed: () {},
87+
child: Text(text)));
88+
89+
// IconButton can't have text; skip
90+
91+
testButtonLetterSpacing('MenuItemButton',
92+
buttonBuilder: (context, text) => MenuItemButton(onPressed: () {},
93+
child: Text(text)));
94+
95+
testButtonLetterSpacing('SubmenuButton',
96+
buttonBuilder: (context, text) => SubmenuButton(menuChildren: const [],
97+
child: Text(text)));
98+
99+
testButtonLetterSpacing('OutlinedButton',
100+
buttonBuilder: (context, text) => OutlinedButton(onPressed: () {},
101+
child: Text(text)));
102+
103+
testButtonLetterSpacing('SegmentedButton',
104+
buttonBuilder: (context, text) => SegmentedButton(selected: const {1},
105+
segments: [ButtonSegment(value: 1, label: Text(text))]));
106+
107+
testButtonLetterSpacing('TextButton',
108+
buttonBuilder: (context, text) => TextButton(onPressed: () {},
109+
child: Text(text)));
110+
});
111+
});
112+
}

0 commit comments

Comments
 (0)