text: Convert the last uses of FontWeight.bold to weightVariableTextStyle
#728
Conversation
chrisbobbe
added
the
integration review
|
Should we give more space to the labels in the profile screen? It seems unnecessary to make not particularly long labels spill onto two lines. |
lib/widgets/profile.dart
Outdated
| static const primaryFieldText = TextStyle(fontSize: 20); | ||
| static const customProfileFieldLabel = TextStyle(fontSize: 15, fontWeight: FontWeight.bold); | ||
| static TextStyle customProfileFieldLabel(BuildContext context) => | ||
| const TextStyle(fontSize: 15) | ||
| .merge(weightVariableTextStyle(context, wght: 700)); | ||
| static const customProfileFieldText = TextStyle(fontSize: 15); |
There was a problem hiding this comment.
nit: blank lines to separate, since these are no longer one line each
| @@ -1351,8 +1360,5 @@ InlineSpan _errorUnimplemented(UnimplementedNode node) { | |||
| } | |||
| } | |||
|
|
|||
| const errorStyle = TextStyle( | |||
There was a problem hiding this comment.
content [nfc]: Move codeStyle to ContentTheme
Do you mean s/codeStyle/errorStyle/?
lib/widgets/content.dart
Outdated
| final errorCodeStyle = kMonospaceTextStyle | ||
| .merge(const TextStyle(fontSize: kBaseFontSize, color: Colors.red)); |
There was a problem hiding this comment.
Maybe move this at the same time? Seems like we'll need to move it too for dark mode; and in any case it's closely related to errorStyle, so it seems like both of them are easier to understand if they're defined next to each other.
Yeah, that'd likely be a good change. We might also try turning the weight down a little on the labels, like perhaps to 600 instead of 700. In general this is a screen that's not yet based on any design from Vlad; I think there wasn't one in the Figma document when Shu was building this screen, so we just made something similar to what zulip-mobile has. So we can make tweaks where we think there are easy improvements, but we don't need to spend time trying to really dial it in. At some point in the future, Vlad will design this screen as a whole, and then we'll revise it to implement that. |
chrisbobbe
force-pushed
the
pr-weight-variable-text-style-last-conversions
branch
from
fe76fae to
ea62232
Compare
Now, this text is styled with a `wght` value of 700, instead of 400 from the ambient style. The FontWeight.bold seems to have already been causing the spans to render more boldly than surrounding text, but in a way that doesn't match what we get for Source Code Pro with `wght` 700. I think this is because of flutter/flutter#136779 . The slightly changed appearance is making some profile field labels run onto two lines on my device on CZO, instead of just one line, as Alya pointed out: zulip#728 (comment) So I'll follow up by giving those labels a bit more horizontal room.
|
|
Thanks! Revision pushed. |
Now, this text is styled with a `wght` value of 700, instead of 400 from the ambient style. The FontWeight.bold seems to have already been causing the spans to render more boldly than surrounding text, but in a way that doesn't match what we get for Source Code Pro with `wght` 700. I think this is because of flutter/flutter#136779 . The slightly changed appearance is making some profile field labels run onto two lines on my device on CZO, instead of just one line, as Alya pointed out: zulip#728 (comment) So I'll follow up by giving those labels a bit more horizontal room.
Now, this text is styled with a `wght` value of 700, instead of 400 from the ambient style. The FontWeight.bold seems to have already been causing the spans to render more boldly than surrounding text, but in a way that doesn't match what we get for Source Code Pro with `wght` 700. I think this is because of flutter/flutter#136779 . We could probably have left out the `const TextStyle().merge()`; I think the only difference it causes is in the resulting style's debugLabel. But I figured we might as well be consistent in how we use weightVariableTextStyle, and in any case it should disappear with flutter/flutter#148026 .
This converts the last remaining place where we were using TextStyle.fontWeight directly, when we should have been using weightVariableTextStyle. (In a future with flutter/flutter#148026 , though, we'll want to change all uses of weightVariableTextStyle back to plain TextStyle.fontWeight.) Now, this text is styled with a `wght` value of 700, instead of 400 from the ambient style. The FontWeight.bold seems to have already been causing the spans to render more boldly than surrounding text, but in a way that doesn't match what we get for Source Code Pro with `wght` 700. I think this is because of flutter/flutter#136779 .
|
Thanks! Looks good — merging. If we want to make further tweaks to the profile screen (like increasing the label's width further, or reducing their font weight), that'll be easy to do as a followup. |
gnprice
force-pushed
the
pr-weight-variable-text-style-last-conversions
branch
from
ea62232 to
3b96ee8
Compare
(That is, the last uses except the ones in code-block styles, which are treated in #727.)