ui: Set letter spacing to 1% in buttons #635

chrisbobbe · 2024-04-25T22:22:55Z

Stacked atop #629. Screenshots coming soon.

Fixes: #548

chrisbobbe · 2024-04-25T22:47:56Z

Seen here with the "Mark all messages as read" button:

Before	After

chrisbobbe · 2024-04-25T23:28:48Z

(There were some tests failing, and I also caught a bug when resolving those; pushed fixes.)

chrisbobbe · 2024-04-25T23:41:31Z

Do reaction chips count as buttons? We can give 1% letter spacing to their label text (which shows either a number or the reacting users' names); that wouldn't be hard.

code

--- lib/widgets/emoji_reaction.dart
+++ lib/widgets/emoji_reaction.dart
@@ -1,5 +1,6 @@
 import 'package:flutter/foundation.dart';
+++ lib/widgets/emoji_reaction.dart
@@ -1,5 +1,6 @@
 import 'package:flutter/foundation.dart';
 import 'package:flutter/material.dart';
+import 'package:zulip/widgets/theme.dart';
 
 import '../api/model/model.dart';
 import '../api/route/messages.dart';
@@ -153,6 +154,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 +170,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,

gnprice

Thanks @chrisbobbe! Comments below.

gnprice · 2024-04-26T22:04:17Z

lib/widgets/app.dart

-          theme: theme,
+          theme: outerThemeData(context),


The commit message says:

theme [nfc]: Pull `outerThemeData` out from app.dart to new theme.dart file

but this makes another change too: it computes this ThemeData from within the builder callback for GlobalStoreWidget.child. Is that intended?

If the method computing this theme data isn't going to be trying to use the store, then it's probably cleanest to keep the computation outside there, with e.g. final theme = outerThemeData(context); at the top of the build method. That way it's clear we compute it once eagerly.

Indeed; thanks for noticing this!

gnprice · 2024-04-26T22:10:19Z

lib/widgets/theme.dart

+    elevatedButtonTheme:  ElevatedButtonThemeData( style: _commonButtonStyle(context)),
+    filledButtonTheme:    FilledButtonThemeData(   style: _commonButtonStyle(context)),
+    iconButtonTheme:      IconButtonThemeData(     style: _commonButtonStyle(context)),


Seems cleanest to factor this common style out as a local — that way we only compute it once.

gnprice · 2024-04-26T22:10:50Z

lib/widgets/theme.dart

+  );
+}
+
+double kButtonTextLetterSpacingProportion = 0.01;


Suggested change

double kButtonTextLetterSpacingProportion = 0.01;

const kButtonTextLetterSpacingProportion = 0.01;

gnprice · 2024-04-26T22:15:15Z

lib/widgets/theme.dart

+
+import 'text.dart';
+
+ThemeData outerThemeData(BuildContext context) {


Is there a coherent description we can give for what the meaning of this "outer ThemeData" is?

It seems like it's basically an intermediate step in the calculation of the "inner ThemeData", which is the theme data we actually want to use in the app.

In that case, can we have one function that just computes the desired ThemeData start to finish, from a single BuildContext? I think that'd make it easier to reason about how the caller should use the function. That function could even have this intermediate step as a ThemeData local variable, if that's helpful.

gnprice · 2024-04-26T22:17:11Z

test/widgets/autocomplete_test.dart

-          child: MessageListPage(
-            narrow: DmNarrow(
-              allRecipientIds: [eg.selfUser.userId, eg.otherUser.userId],
-              selfUserId: eg.selfUser.userId,
-            ))))));
+  await tester.pumpWidget(const ZulipApp());
+  await tester.pump();
+  final navigator = await ZulipApp.navigator;
+  navigator.push(MessageListPage.buildRoute(accountId: eg.selfAccount.id,
+    narrow: DmNarrow(


These rearrangements of existing tests are probably clearest as a separate prep commit.

gnprice · 2024-04-26T22:18:09Z

test/widgets/autocomplete_test.dart

+    )));

  // global store, per-account store, and message list get loaded
  await tester.pumpAndSettle();


This comment is no longer accurate. Perhaps just cut it:

Suggested change

)));

// global store, per-account store, and message list get loaded

await tester.pumpAndSettle();

)));

await tester.pumpAndSettle();

And perhaps it can be reduced to just a pump?

gnprice · 2024-04-26T22:39:47Z

lib/widgets/message_list.dart

+                  // Restate [FilledButton]'s default…
+                  themeData.textTheme.labelLarge!.merge(filledButtonThemeTextStyle)


Huh.

Does this mean we could just set .textTheme.labelLarge on our main ThemeData, and skip reaching into all the individual button styles? That seems like it'd be cleaner.

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.

That's a good idea! Done.

gnprice · 2024-04-26T22:40:18Z

lib/widgets/app.dart

@@ -109,37 +109,6 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {

  @override
  Widget build(BuildContext context) {
-    final theme = ThemeData(


Cool, good to get this pulled out of ZulipApp.

chrisbobbe · 2024-04-27T00:55:52Z

Thanks for the review! Revision pushed, with the simpler approach that you outlined in #635 (comment) and that we talked about in the office.

gnprice

Thanks! This all looks good; just small comments below.

(I'll review #629 separately, but it also looks good modulo small comments.)

gnprice · 2024-04-29T20:25:39Z

test/widgets/theme_test.dart

+              return Builder(builder: (context) =>
+                buttonBuilder(context, buttonText));


Suggested change

return Builder(builder: (context) =>

buttonBuilder(context, buttonText));

return buttonBuilder(context, buttonText);

This would have the same effect, right? It doesn't seem like this build context is effectively any different from its parent.

gnprice · 2024-04-29T20:33:19Z

test/widgets/theme_test.dart

+        double? expectedFontSize;
+        double? expectedLetterSpacing;


Can tighten up expectations here slightly:

Suggested change

double? expectedFontSize;

double? expectedLetterSpacing;

late final double expectedFontSize;

late final double expectedLetterSpacing;

That way it's clear (and enforced) that these only get set once, and that they always get set before they're read.

gnprice · 2024-04-29T20:37:01Z

test/widgets/theme_test.dart

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


It seems like none of these builders actually use the context. So they can be simplified to just widgets (by pulling up the buttonText constant too):

Suggested change

buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},

child: Text(text)));

button: ElevatedButton(onPressed: () {}, child: const Text(buttonText)));

Ah right, cool!

…t file

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

chrisbobbe · 2024-04-30T01:09:08Z

Thanks! Revision pushed.

gnprice · 2024-04-30T23:07:57Z

Thanks! LGTM; merging.

chrisbobbe added the a-design Visual and UX design label Apr 25, 2024

chrisbobbe requested a review from gnprice April 25, 2024 22:22

chrisbobbe force-pushed the pr-letter-spacing-1% branch from 03d16b8 to 562bd17 Compare April 25, 2024 22:45

chrisbobbe force-pushed the pr-letter-spacing-1% branch from 562bd17 to 3f822eb Compare April 25, 2024 23:28

chrisbobbe force-pushed the pr-letter-spacing-1% branch from 3f822eb to f7669ea Compare April 25, 2024 23:31

gnprice reviewed Apr 26, 2024

View reviewed changes

chrisbobbe force-pushed the pr-letter-spacing-1% branch from f7669ea to 71949f1 Compare April 27, 2024 00:55

gnprice reviewed Apr 29, 2024

View reviewed changes

chrisbobbe added 2 commits April 29, 2024 18:03

theme [nfc]: Pull zulipThemeData out from app.dart to new theme.dar…

af20651

…t file

chrisbobbe force-pushed the pr-letter-spacing-1% branch from 71949f1 to d882f50 Compare April 30, 2024 01:09

gnprice merged commit d882f50 into zulip:main Apr 30, 2024

chrisbobbe deleted the pr-letter-spacing-1% branch April 30, 2024 23:08

chrisbobbe mentioned this pull request May 3, 2024

model: Track/memoize stream-color variants somewhere other than /api/model #393

Closed

	double kButtonTextLetterSpacingProportion = 0.01;
	const kButtonTextLetterSpacingProportion = 0.01;


		import 'text.dart';

		ThemeData outerThemeData(BuildContext context) {

		// Restate [FilledButton]'s default…
		themeData.textTheme.labelLarge!.merge(filledButtonThemeTextStyle)

		return Builder(builder: (context) =>
		buttonBuilder(context, buttonText));

	return Builder(builder: (context) =>
	buttonBuilder(context, buttonText));
	return buttonBuilder(context, buttonText);

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

	buttonBuilder: (context, text) => ElevatedButton(onPressed: () {},
	child: Text(text)));
	button: ElevatedButton(onPressed: () {}, child: const Text(buttonText)));

ui: Set letter spacing to 1% in buttons #635

ui: Set letter spacing to 1% in buttons #635

Uh oh!

Conversation

chrisbobbe commented Apr 25, 2024

Uh oh!

chrisbobbe commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

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!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Apr 27, 2024

Uh oh!

gnprice left a comment

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!

chrisbobbe commented Apr 30, 2024

Uh oh!

gnprice commented Apr 30, 2024

Uh oh!

Uh oh!

chrisbobbe commented Apr 25, 2024 •

edited

Loading

chrisbobbe commented Apr 25, 2024 •

edited

Loading

chrisbobbe commented Apr 25, 2024 •

edited

Loading