Set letter spacing to 1% in buttons #548

gnprice · 2024-03-05T21:10:19Z

This is a followup to #545. That issue is focused on the one most conspicuous kind of text in the app; this one is for the rest of the app.

Specifically we want:

letter spacing of 1% (i.e., 0.01 times fontSize) in buttons;
~~letterSpacing: 0 everywhere else, except where the Figma design specifies otherwise.~~ (This part was handled in text: Set 0 letter spacing throughout zulipTypography #549.)

Chat thread.

The text was updated successfully, but these errors were encountered:

Gosplex · 2024-03-06T23:26:43Z

I am interested in working on this issue
So I have created a branch and forked the repo
I have also started implementing the letter spacing on the buttons throughout the app

gnprice · 2024-03-07T00:53:00Z

Hi @Gosplex, welcome.

I think we're not going to want to do this by setting letterSpacing individually on each of the buttons in the app, or on each of the other pieces of text. That would make it error-prone because it'd be easy to forget to include it in a given place when adding a new piece of UI, and it'd also make for additional noise when reading the code for each place text appears.

Instead, we'll use the "theme" features of Flutter's Material Design implementation. See TextTheme and related docs:
https://main-api.flutter.dev/flutter/material/TextTheme-class.html
https://main-api.flutter.dev/flutter/material/Typography-class.html

Your first step will be to find the existing code in the app that works with those APIs, and start to get an idea of how you'd approach addressing this issue there.

There's also several button-related "theme" classes, which control the text (and other things) in buttons specifically. So you'll want to find those and get an idea of how you'd address the buttons part of this issue using those.

If you have any specific questions as you're exploring those, please go ahead and ask. Making a new thread in #mobile-team would be a good venue for that.

Gosplex · 2024-03-07T00:55:08Z

Yes Sure!
Duly noted!
I would give feedback about this sonnest

This zeroes out the letter spacing in message content (zulip#545), and also everywhere else that uses styles from zulipTypography without overriding the letter spacing. It's worth mentioning that letter spacing is one of the parameters that are carefully chosen for the 15 TextThemes that make up the default Typography in Material 3: {display,headline,title,label,body} × {small,medium,large} So this might be considered a regression...if the Material spec were our primary or only source of truth for text styles. But it isn't: we have a designer (Vlad), and this change brings us closer to his specified design. See zulip#545 and zulip#548. I also checked to see if the Material 3 default varies the letter spacing among the three [ScriptCategory]ies (englishLike, dense, and tall), which might be a reason to ask Vlad if we want to do the same. But the letter-spacing values are the same across the [ScriptCategory]ies, so no need for that. After this, there's still more to do for zulip#548: - letter spacing of 1% (i.e., 0.01 times `fontSize`) in buttons - perhaps audit for UI code that's meant to follow Figma already, but where we failed to set a non-zero letter spacing that was specified in Figma Fixes: zulip#545 Fixes-partly: zulip#548

Gosplex · 2024-03-07T16:48:35Z

Good day
So I have carefully gone through the codebase and the help material provided by @gnprice and I found out that the ThemeData of the app is managed at app.dart file. So any changes made there would be made available throughout the app

This zeroes out the letter spacing in message content (zulip#545), and also everywhere else that uses styles from zulipTypography without overriding the letter spacing. It's worth mentioning that letter spacing is one of the parameters that are carefully chosen for the 15 TextThemes that make up the default Typography in Material 3: {display,headline,title,label,body} × {small,medium,large} So this might be considered a regression...if the Material spec were our primary or only source of truth for text styles. But it isn't: we have a designer (Vlad), and this change brings us closer to his specified design. See zulip#545 and zulip#548. I also checked to see if the Material 3 default varies the letter spacing among the three [ScriptCategory]ies (englishLike, dense, and tall), which might be a reason to ask Vlad if we want to do the same. But the letter-spacing values are the same across the [ScriptCategory]ies, so no need for that. After this, there's still more to do for zulip#548: letter spacing of 1% (i.e., 0.01 times `fontSize`) in buttons. Fixes: zulip#545 Fixes-partly: zulip#548

gnprice · 2024-03-08T01:57:33Z

Cool, please go ahead. Note that part of this issue is now done, so the remaining part is the part about 1% letter spacing in buttons.

For an example of how we adjust themes systematically, take a look at #549 and the file that PR modified. We'll need to do something similar for the button themes.

Gosplex · 2024-03-08T09:40:39Z

@gnprice Sir
I have created a pull request for this issue.
Please review.
Thanks a bunch

Fixes: zulip#548

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

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. Empirically, the mark-all-as-read button (a [FilledButton.icon]) ignores the TextStyle in the ambient FilledButtonThemeData unless explicitly told to use it; that's because we use the `textStyle` param. So, explicitly merge that in, and adjust some tests (action_sheet_test, autocomplete_test, message_list_test) that weren't providing the FilledButtonThemeData so that they do provide it. ...hmm, but for that specific button, the Figma asks for a specific font size, which differs from the default `labelLarge`. That means we need to set `letterSpacing` right alongside `fontSize`, because the letter spacing should be proportional to the font size. Having done so, the restatement of the TextStyle in FilledButtonThemeData is now completely redundant. Shrug...I guess... if we add to those theme styles, we'll want them to be merged in, as done here, and that seems really easy to forget to do. Fixes: zulip#548

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

gnprice added a-design Visual and UX design beta feedback Things beta users have specifically asked for labels Mar 5, 2024

gnprice added this to the Beta 2 milestone Mar 5, 2024

github-project-automation bot added this to Flutter app Mar 5, 2024

gnprice mentioned this issue Mar 5, 2024

Zero out letter spacing in message content #545

Closed

chrisbobbe mentioned this issue Mar 7, 2024

text: Set 0 letter spacing throughout zulipTypography #549

Merged

gnprice changed the title ~~Set letter spacing throughout app~~ Set letter spacing to 1% in buttons Mar 8, 2024

gnprice assigned Gosplex Mar 8, 2024

gnprice mentioned this issue Mar 8, 2024

Added letterSpacing to buttons #557

Closed

chrisbobbe mentioned this issue Apr 24, 2024

text: Add and use proportionalLetterSpacing #629

Merged

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Apr 25, 2024

ui: Set letter spacing to 1% in buttons

7c6b088

Fixes: zulip#548

chrisbobbe mentioned this issue Apr 25, 2024

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

Merged

gnprice closed this as completed in d882f50 Apr 30, 2024

github-project-automation bot moved this to Done in Flutter app Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set letter spacing to 1% in buttons #548

Set letter spacing to 1% in buttons #548

gnprice commented Mar 5, 2024 •

edited

Loading

Gosplex commented Mar 6, 2024 •

edited

Loading

gnprice commented Mar 7, 2024 •

edited

Loading

Gosplex commented Mar 7, 2024

Gosplex commented Mar 7, 2024

gnprice commented Mar 8, 2024

Gosplex commented Mar 8, 2024

Set letter spacing to 1% in buttons #548

Set letter spacing to 1% in buttons #548

Comments

gnprice commented Mar 5, 2024 • edited Loading

Gosplex commented Mar 6, 2024 • edited Loading

gnprice commented Mar 7, 2024 • edited Loading

Gosplex commented Mar 7, 2024

Gosplex commented Mar 7, 2024

gnprice commented Mar 8, 2024

Gosplex commented Mar 8, 2024

gnprice commented Mar 5, 2024 •

edited

Loading

Gosplex commented Mar 6, 2024 •

edited

Loading

gnprice commented Mar 7, 2024 •

edited

Loading