theme: Add ThemeExtension for Figma design variables; add/use two more variables in AppBar
#734
Conversation
chrisbobbe
added
the
integration review
|
In this commit: the I think the change is probably OK, and possibly an improvement (in fact that particular app bar might do better with
|
|
Ah hmm, I wonder if it's worth polishing the message list app bar, which I realized is also affected by these changes. I think it looks a bit worse now, but I don't think there's a ready Figma design for it yet. —ah, looks like basically a worsening of something already flagged in the code: // TODO(design): The vertical alignment of the stream privacy icon is a bit ad hoc.
// For screenshots of some experiments, see:
// https://github.com/zulip/zulip-flutter/pull/219#discussion_r1281024746
|
I've added a commit that does this: It does change the vertical alignment from baseline to center. One concern that came up in the discussion linked from the code comment (#219 (comment) ) was that choosing "center" would make the icon look too low. But I don't think it looks too low now (see screenshots below); maybe one factor is our change of font since that older discussion? Screenshots shown after the new commit, with different values of the system text-size setting:
|
|
I guess the icon starts looking too small in the screenshots on the right; I don't know how fixable that is. |
Hmm, yeah; the icon is closely associated with the text, so probably it should scale with it, instead of always being the same size? There's a setting for that; it should be easy: https://api.flutter.dev/flutter/widgets/Icon/applyTextScaling.html This could be done separately as a followup, I think. The icon in |
There was a problem hiding this comment.
Thanks @chrisbobbe! This all LGTM; one nit below, and please go ahead and merge.
I think the behavior shown in the screenshots where the icon doesn't grow when the text is big is actually fine — it's normal and helpful that increasing text size doesn't cause the rest of the UI to expand, other than text. See discussion here:
https://github.com/zulip/zulip-mobile/blob/e352f563ecf2fa9b09b688d5a65b6bc89b0358bc/docs/background/webview.md#why-mostly-px-and-not-rem
Arguably these particular icons are more text-like (like a kind of emoji) and so should scale with the text size after all. But I don't think we need to worry about it.
lib/widgets/theme.dart
Outdated
| titleTextStyle: TextStyle( | ||
| inherit: false, | ||
|
|
||
| color: designVariables.title, |
There was a problem hiding this comment.
nit:
| titleTextStyle: TextStyle( | |
| inherit: false, | |
| color: designVariables.title, | |
| titleTextStyle: TextStyle( | |
| inherit: false, | |
| color: designVariables.title, |
| /// For how to export these from the Figma, see: | ||
| /// https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=2945-49492&t=MEb4vtp7S26nntxm-0 |
This seems like a handy function to have around; there were a handful of places that were wanting something like this. In b75908b, we aimed to make ContentTheme.textStylePlainParagraph a very direct, NFC translation of some Typography defaults, but we accidentally fell short a little bit: I had forgotten that we support Japanese, and that commit just hardcoded TextBaseline.alphabetic, which was a regression. Since the "geometry TextThemes" for M3 only differ in `textBaseline` [1], I believe that commit would have been a pure translation if we had used this new function. (We did follow up with 6284ed5, removing the textBaseline field altogether. But that put the style at odds with text basically everywhere else, which inherits a textBaseline from Typography.) I was hoping to see an improvement in how Japanese text is rendered after this change, but I wasn't able to; I didn't notice any change in appearance between TextBaseline.alphabetic and TextBaseline.ideographic (or a TextStyle that omits the field, in the case of ContentTheme.textStylePlainParagraph.) Anyway, if this is because of incomplete support for `ideographic`, at least we'll be well-prepared to take improvements as they arrive. Someone did post some screenshots with both values rendering identically: flutter/flutter#94680 [1] The Material 2 spec apparently called for more differences that were removed in Material 3; see the docs for Typography.dense2021 and tall2021.
Much like we did in b75908b for ContentTheme.textStylePlainParagraph, this is meant to collect all the details that had been implicitly defining the app-bar title style, from various sources: - [_M3Typography.englishLike] or [_M3Typography.dense] (the latter for Japanese) - [Typography.blackCupertino] or [Typography.blackMountainView] (depending on platform) - `zulipTypography` in lib/widgets/text.dart - [ColorScheme.onSurface] , make them explicit, and make it clear that this doesn't inherit from anything. (Unlike in b75908b, this handles the wrinkle of giving the appropriate textBaseline for our Japanese support. See a recent commit that added localizedTextBaseline.) If we were defining this within the scope of the global `ThemeData`, rather than outside it (to add to it), we might instead have aliased the details like so: final theme = Theme.of(context); theme.textTheme.titleLarge! .apply(color: theme.colorScheme.onSurface); , which is much simpler; oh, well. That `textTheme` is the result of complicated computations based on Typography in [Theme.of], and [Theme.of] only works when you're below the right [Theme] in the widget tree. Next, we'll change the `color`, `fontSize`, and `height` to align with the Figma. (We couldn't have just passed a `TextStyle` with those three fields by themselves, even with `inherit: true`. If you give AppBar a TextStyle for the title like this, it just takes it literally, in place of the default style that applies when you don't give the TextStyle.)
Exciting to see this marked "ready for dev"! https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=2957-50895&m=dev
I don't expect these to be fundamentally hard, they're just not really trivial; we'll leave them for later.
It's possible that there's a different organization we may end up preferring -- for example, if we find ourselves wanting to create our own variables that are not in the Figma design. But this seems like a helpful thing to try, for paving the way to implement the Figma design.
…ange In a recent commit in this series, we decreased the app-bar title text size, to align with the new Figma. The spacing and alignment in MessageListAppBarTitle for stream and topic narrows looked a little off after doing that. This should make it look better. I'm leaving the comment saying it's "a bit ad hoc", though; this will remain true until we have a Figma design to follow.
chrisbobbe
force-pushed
the
pr-design-variables
branch
from
ea91750 to
3a7ddc3
Compare
|
Thanks! Done. |
Yeah; true. Maybe worth mentioning that we can apply custom scalers where we want to: for example, if we wanted to let icons respond to the text-size setting only when it's set smaller than the default 1x. But I agree not a priority for now. |
It seems helpful to have a central place to put the design variables we export from the Figma design, to help pave the way to implementing it. This PR adds such a place, as a
ThemeExtension.It also takes two more color variables from the Figma (
titleandicon), to add to the three we've already been using, and applies them to the app bar, since that's something we see marked "ready for dev": https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=2957-50895&m=devWhile we're at it, this also applies the Figma's specified font weight, size, and line height to the app-bar title. (Those aren't expressed as variables in the Figma.)
There's still more to do to align the app bar with what we see in the Figma, and I add TODOs about some of this.