Skip to content

Commit

Permalink
ui: Set letter spacing to 1% in buttons
Browse files Browse the repository at this point in the history
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 zulip#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
> zulip#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: zulip#548
  • Loading branch information
chrisbobbe committed Apr 30, 2024
1 parent af20651 commit d882f50
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 4 deletions.
7 changes: 6 additions & 1 deletion lib/widgets/emoji_reaction.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
10 changes: 10 additions & 0 deletions lib/widgets/text.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions test/flutter_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,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
7 changes: 5 additions & 2 deletions test/widgets/text_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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', () {
Expand Down
71 changes: 71 additions & 0 deletions test/widgets/theme_test.dart
Original file line number Diff line number Diff line change
@@ -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<void> 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<RenderParagraph>(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)));
});
}

0 comments on commit d882f50

Please sign in to comment.