From d882f50bf7d917ce7e040d7ee1f66f1d803ef988 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 26 Apr 2024 17:14:06 -0700 Subject: [PATCH] ui: Set letter spacing to 1% in buttons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TextTheme.labelLarge is the default label style in all the various Material buttons; see: [ElevatedButton.defaultStyleOf], [FilledButton.defaultStyleOf], [MenuItemButton.defaultStyleOf], [MenuItemButton.defaultStyleOf], [OutlinedButton.defaultStyleOf], `defaults` in [SegmentedButton.build], [TextButton.defaultStyleOf] . Since our buttons are all Material buttons, changing this will effectively "set letter spacing to 1% in buttons", which is #548. (...Actually, I guess there's one kind of button that's not a Material button: reaction chips in the message list. So, adjust those separately, also in this commit.) Greg mentioned, when suggesting this approach, > In principle that has a slightly broader effect than specified in > #548, in that it affects any other Material widgets that use the > `labelLarge` text style. But fundamentally those are widgets that > the Material designers intended to have a similar text style to > that of buttons (even though they're not called "buttons"), so > it's likely that that'll be the right answer anyway. > If we end up with such a widget where Vlad wants a different > spacing, we can address that then… and I think it's fairly likely > that in that case he'd want something that wasn't the Material > widget in any event. Which makes sense and works for me. Fixes: #548 --- lib/widgets/emoji_reaction.dart | 7 +++- lib/widgets/message_list.dart | 4 +- lib/widgets/text.dart | 10 +++++ test/flutter_checks.dart | 1 + test/widgets/text_test.dart | 7 +++- test/widgets/theme_test.dart | 71 +++++++++++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 test/widgets/theme_test.dart diff --git a/lib/widgets/emoji_reaction.dart b/lib/widgets/emoji_reaction.dart index ba7fc7a4a4..2c63fef9c4 100644 --- a/lib/widgets/emoji_reaction.dart +++ b/lib/widgets/emoji_reaction.dart @@ -153,6 +153,7 @@ class ReactionChip extends StatelessWidget { // which we learn about especially late). final maxLabelWidth = (maxRowWidth - 6) * 0.75; // 6 is padding + final labelScaler = _labelTextScalerClamped(context); return Row( mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.center, @@ -168,9 +169,13 @@ class ReactionChip extends StatelessWidget { constraints: BoxConstraints(maxWidth: maxLabelWidth), child: Text( textWidthBasis: TextWidthBasis.longestLine, - textScaler: _labelTextScalerClamped(context), + textScaler: labelScaler, style: TextStyle( fontSize: (14 * 0.90), + letterSpacing: proportionalLetterSpacing(context, + kButtonTextLetterSpacingProportion, + baseFontSize: (14 * 0.90), + textScaler: labelScaler), height: 13 / (14 * 0.90), color: labelColor, ).merge(weightVariableTextStyle(context, diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index a102fb13e5..f3c8b63ba0 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -484,8 +484,10 @@ class MarkAsReadWidget extends StatelessWidget { // [zulipTypography]… Theme.of(context).textTheme.labelLarge! // …then clobber some attributes to follow Figma: - .merge(const TextStyle( + .merge(TextStyle( fontSize: 18, + letterSpacing: proportionalLetterSpacing(context, + kButtonTextLetterSpacingProportion, baseFontSize: 18), height: (23 / 18)) .merge(weightVariableTextStyle(context, wght: 400))), shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), diff --git a/lib/widgets/text.dart b/lib/widgets/text.dart index a3c82aceb9..d0fbea3546 100644 --- a/lib/widgets/text.dart +++ b/lib/widgets/text.dart @@ -40,6 +40,14 @@ Typography zulipTypography(BuildContext context) { result = _convertTextTheme(result, (maybeInputStyle, _) => maybeInputStyle?.merge(const TextStyle(letterSpacing: 0))); + result = result.copyWith( + labelLarge: result.labelLarge!.copyWith( + fontSize: 14.0, // (should be unchanged; restated here for explicitness) + letterSpacing: proportionalLetterSpacing(context, + kButtonTextLetterSpacingProportion, baseFontSize: 14.0), + ), + ); + return result; } @@ -167,6 +175,8 @@ final TextStyle kMonospaceTextStyle = TextStyle( inherit: true, ); +const kButtonTextLetterSpacingProportion = 0.01; + /// A mergeable [TextStyle] to use when the preferred font has a "wght" axis. /// /// Some variable fonts can be controlled on a "wght" axis. diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index bc7e9d83a7..20a38225dc 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -65,6 +65,7 @@ extension TextFieldChecks on Subject { extension TextStyleChecks on Subject { Subject get inherit => has((t) => t.inherit, 'inherit'); + Subject get fontSize => has((t) => t.fontSize, 'fontSize'); Subject get fontWeight => has((t) => t.fontWeight, 'fontWeight'); Subject get letterSpacing => has((t) => t.letterSpacing, 'letterSpacing'); Subject?> get fontVariations => has((t) => t.fontVariations, 'fontVariations'); diff --git a/test/widgets/text_test.dart b/test/widgets/text_test.dart index 4a05bd1f3c..56324b11f9 100644 --- a/test/widgets/text_test.dart +++ b/test/widgets/text_test.dart @@ -54,11 +54,14 @@ void main() { }); } - testWidgets('zero letter spacing', (tester) async { + testWidgets('letter spacing', (tester) async { check(await getZulipTypography(tester, platformRequestsBold: false)) ..englishLike.bodyMedium.isNotNull().letterSpacing.equals(0) + ..englishLike.labelLarge.isNotNull().letterSpacing.equals(0.14) ..dense.bodyMedium.isNotNull().letterSpacing.equals(0) - ..tall.bodyMedium.isNotNull().letterSpacing.equals(0); + ..dense.labelLarge.isNotNull().letterSpacing.equals(0.14) + ..tall.bodyMedium.isNotNull().letterSpacing.equals(0) + ..tall.labelLarge.isNotNull().letterSpacing.equals(0.14); }); test('Typography has the assumed fields', () { diff --git a/test/widgets/theme_test.dart b/test/widgets/theme_test.dart new file mode 100644 index 0000000000..8bc32e94d9 --- /dev/null +++ b/test/widgets/theme_test.dart @@ -0,0 +1,71 @@ +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('button text size and letter spacing', () { + const buttonText = 'Zulip'; + + Future doCheck( + String description, { + required Widget button, + double? ambientTextScaleFactor, + }) async { + testWidgets(description, (WidgetTester tester) async { + if (ambientTextScaleFactor != null) { + tester.platformDispatcher.textScaleFactorTestValue = ambientTextScaleFactor; + addTearDown(tester.platformDispatcher.clearTextScaleFactorTestValue); + } + late final double expectedFontSize; + late final double expectedLetterSpacing; + await tester.pumpWidget( + Builder(builder: (context) => MaterialApp( + theme: zulipThemeData(context), + home: Builder(builder: (context) { + expectedFontSize = Theme.of(context).textTheme.labelLarge!.fontSize!; + expectedLetterSpacing = proportionalLetterSpacing(context, + 0.01, baseFontSize: expectedFontSize); + return button; + })))); + + final text = tester.renderObject(find.text(buttonText)).text; + check(text.style!) + ..fontSize.equals(expectedFontSize) + ..letterSpacing.equals(expectedLetterSpacing); + }); + } + + doCheck('with device text size adjusted', + ambientTextScaleFactor: 2.0, + button: ElevatedButton(onPressed: () {}, child: const Text(buttonText))); + + doCheck('ElevatedButton', + button: ElevatedButton(onPressed: () {}, child: const Text(buttonText))); + + doCheck('FilledButton', + button: FilledButton(onPressed: () {}, child: const Text(buttonText))); + + // IconButton can't have text; skip + + doCheck('MenuItemButton', + button: MenuItemButton(onPressed: () {}, child: const Text(buttonText))); + + doCheck('SubmenuButton', + button: const SubmenuButton(menuChildren: [], child: Text(buttonText))); + + doCheck('OutlinedButton', + button: OutlinedButton(onPressed: () {}, child: const Text(buttonText))); + + doCheck('SegmentedButton', + button: SegmentedButton(selected: const {1}, + segments: const [ButtonSegment(value: 1, label: Text(buttonText))])); + + doCheck('TextButton', + button: TextButton(onPressed: () {}, child: const Text(buttonText))); + }); +}