Skip to content

Commit

Permalink
ui: Set letter spacing to 1% in buttons
Browse files Browse the repository at this point in the history
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: zulip#548
  • Loading branch information
chrisbobbe committed Apr 25, 2024
1 parent dd97594 commit 562bd17
Show file tree
Hide file tree
Showing 5 changed files with 162 additions and 4 deletions.
3 changes: 2 additions & 1 deletion lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
(_) => widget._declareReady());
}
GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
return child!;
return Theme(data: innerThemeData(context),
child: child!);
},

// We use onGenerateInitialRoutes for the real work of specifying the
Expand Down
10 changes: 7 additions & 3 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,11 @@ class MarkAsReadWidget extends StatelessWidget {

@override
Widget build(BuildContext context) {
final themeData = Theme.of(context);
assert(themeData.filledButtonTheme.style?.textStyle != null);
final filledButtonThemeTextStyle =
themeData.filledButtonTheme.style?.textStyle!.resolve({});
assert(filledButtonThemeTextStyle != null);
final zulipLocalizations = ZulipLocalizations.of(context);
final store = PerAccountStoreWidget.of(context);
final unreadCount = store.unreads.countInNarrow(narrow);
Expand All @@ -480,9 +485,8 @@ class MarkAsReadWidget extends StatelessWidget {
backgroundColor: _UnreadMarker.color,
minimumSize: const Size.fromHeight(38),
textStyle:
// Restate [FilledButton]'s default, which inherits from
// [zulipTypography]…
Theme.of(context).textTheme.labelLarge!
// Restate [FilledButton]'s default…
themeData.textTheme.labelLarge!.merge(filledButtonThemeTextStyle)
// …then clobber some attributes to follow Figma:
.merge(const TextStyle(
fontSize: 18,
Expand Down
39 changes: 39 additions & 0 deletions lib/widgets/theme.dart
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,42 @@ ThemeData outerThemeData(BuildContext context) {
/// This is chosen as the sRGB midpoint of the Zulip logo's gradient.
// As computed by Anders: https://github.com/zulip/zulip-mobile/pull/4467
const kZulipBrandColor = Color.fromRGBO(0x64, 0x92, 0xfe, 1);

/// A [ThemeData] that depends on [outerThemeData] and copies it with additions.
///
/// [context] must be below a [Theme] that provides [outerThemeData],
/// so that this can use [Theme.of] to synthesize a [ThemeData.textTheme]
/// based on [outerThemeData]'s [ThemeData.typography].
// TODO(upstream) nicer if this could return a [ThemeData] with just the added
// fields, instead of having to call `copyWith` on `Theme.of(context)`.
// Then callers could call a hypothetical ThemeData.merge method with it.
// But such a method doesn't exist yet:
// https://github.com/flutter/flutter/issues/43823
ThemeData innerThemeData(BuildContext context) {
final theme = Theme.of(context);
assert(theme.textTheme.labelLarge!.debugLabel!.contains('zulipTypography'));
return theme.copyWith(
elevatedButtonTheme: ElevatedButtonThemeData( style: _commonButtonStyle(context)),
filledButtonTheme: FilledButtonThemeData( style: _commonButtonStyle(context)),
iconButtonTheme: IconButtonThemeData( style: _commonButtonStyle(context)),
menuButtonTheme: MenuButtonThemeData( style: _commonButtonStyle(context)),
outlinedButtonTheme: OutlinedButtonThemeData( style: _commonButtonStyle(context)),
segmentedButtonTheme: SegmentedButtonThemeData(style: _commonButtonStyle(context)),
textButtonTheme: TextButtonThemeData( style: _commonButtonStyle(context)),
);
}

ButtonStyle _commonButtonStyle(BuildContext context) {
// labelLarge is the default for all kinds of buttons that can have text. See:
// [ElevatedButton.defaultStyleOf], [FilledButton.defaultStyleOf],
// [MenuItemButton.defaultStyleOf], [MenuItemButton.defaultStyleOf],
// [OutlinedButton.defaultStyleOf], `defaults` in [SegmentedButton.build],
// and [TextButton.defaultStyleOf].
final fontSize = Theme.of(context).textTheme.labelLarge!.fontSize!;
return ButtonStyle(
textStyle: WidgetStatePropertyAll(TextStyle(
// If changing `fontSize` or `letterSpacing`, change the other too.
fontSize: fontSize,
letterSpacing: proportionalLetterSpacing(context,
0.01, baseFontSize: fontSize))));
}
2 changes: 2 additions & 0 deletions test/flutter_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ extension ValueNotifierChecks<T> on Subject<ValueNotifier<T>> {

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

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

extension TextStyleChecks on Subject<TextStyle> {
Subject<bool> get inherit => has((t) => t.inherit, 'inherit');
Subject<double?> get fontSize => has((t) => t.fontSize, 'fontSize');
Subject<FontWeight?> get fontWeight => has((t) => t.fontWeight, 'fontWeight');
Subject<double?> get letterSpacing => has((t) => t.letterSpacing, 'letterSpacing');
Subject<List<FontVariation>?> get fontVariations => has((t) => t.fontVariations, 'fontVariations');
Expand Down
112 changes: 112 additions & 0 deletions test/widgets/theme_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import 'package:checks/checks.dart';
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:zulip/widgets/text.dart';
import 'package:zulip/widgets/theme.dart';

import '../flutter_checks.dart';

void main() {
group('innerThemeData', () {
testWidgets('smoke', (tester) async {
ThemeData? outerThemeDataValue;
ThemeData? value;
await tester.pumpWidget(
Builder(builder: (context) => MaterialApp(
theme: outerThemeData(context),
home: Builder(builder: (context) {
outerThemeDataValue = Theme.of(context);
return Theme(
data: innerThemeData(context),
child: Builder(builder: (context) {
value = Theme.of(context);
return const Placeholder();
}));
}))));

final outerElevatedButtonLetterSpacing = outerThemeDataValue
!.elevatedButtonTheme.style?.textStyle?.resolve({})?.letterSpacing;
final elevatedButtonLetterSpacing = value
!.elevatedButtonTheme.style?.textStyle?.resolve({})?.letterSpacing;
check(outerElevatedButtonLetterSpacing).isNull();
check(elevatedButtonLetterSpacing).isNotNull();

// innerThemeData should extend outerThemeData using [ThemeData.copyWith]
// (at least for now, lacking a ThemeData.merge method). Pick a value that
// we set in outerThemeData and check that it hasn't been dropped.
check(value!.scaffoldBackgroundColor)
..equals(outerThemeDataValue!.scaffoldBackgroundColor)
..equals(const Color(0xfff6f6f6));
});

group('button text size and letter spacing', () {
Future<void> testButtonLetterSpacing(
String description, {
required Widget Function(BuildContext context, String text) buttonBuilder,
double? ambientTextScaleFactor,
}) async {
testWidgets(description, (WidgetTester tester) async {
if (ambientTextScaleFactor != null) {
tester.platformDispatcher.textScaleFactorTestValue = ambientTextScaleFactor;
addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue);
}
const buttonText = 'Zulip';
double? expectedFontSize;
double? expectedLetterSpacing;
await tester.pumpWidget(
Builder(builder: (context) => MaterialApp(
theme: outerThemeData(context),
home: Builder(builder: (context) {
expectedFontSize = Theme.of(context).textTheme.labelLarge!.fontSize!;
expectedLetterSpacing = proportionalLetterSpacing(context,
0.01, baseFontSize: expectedFontSize!);
return Theme(data: innerThemeData(context),
child: Builder(builder: (context) =>
buttonBuilder(context, buttonText)));
}))));

final text = tester.renderObject<RenderParagraph>(find.text(buttonText)).text;
check(text.style!)
..fontSize.equals(expectedFontSize)
..letterSpacing.equals(expectedLetterSpacing);
});
}

testButtonLetterSpacing('with device text size adjusted',
ambientTextScaleFactor: 2.0,
buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},
child: Text(text)));

testButtonLetterSpacing('ElevatedButton',
buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},
child: Text(text)));

testButtonLetterSpacing('FilledButton',
buttonBuilder: (context, text) => FilledButton(onPressed: () {},
child: Text(text)));

// IconButton can't have text; skip

testButtonLetterSpacing('MenuItemButton',
buttonBuilder: (context, text) => MenuItemButton(onPressed: () {},
child: Text(text)));

testButtonLetterSpacing('SubmenuButton',
buttonBuilder: (context, text) => SubmenuButton(menuChildren: const [],
child: Text(text)));

testButtonLetterSpacing('OutlinedButton',
buttonBuilder: (context, text) => OutlinedButton(onPressed: () {},
child: Text(text)));

testButtonLetterSpacing('SegmentedButton',
buttonBuilder: (context, text) => SegmentedButton(selected: const {1},
segments: [ButtonSegment(value: 1, label: Text(text))]));

testButtonLetterSpacing('TextButton',
buttonBuilder: (context, text) => TextButton(onPressed: () {},
child: Text(text)));
});
});
}

0 comments on commit 562bd17

Please sign in to comment.