Skip to content

content: Use 17 for base font size, and fix code/mentions/times in headings #540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

content: Use 17 for base font size, and fix code/mentions/times in headings #540

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75eb29e
content [nfc]: Pass surrounding span style to some elements that want it
chrisbobbe Feb 29, 2024
0e2e34e
content: Show code, mentions, and times properly even in headings, etc.
chrisbobbe Feb 29, 2024
09a8695
content [nfc]: Set error{,Code}Style font size to kBaseFontSize expli…
chrisbobbe Feb 28, 2024
d555bb9
content: For headings, use weightVariableTextStyle
chrisbobbe Feb 28, 2024
2f0af19
content: Use 17 for base font size (and height 22), like in Figma
chrisbobbe Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 48 additions & 25 deletions lib/widgets/content.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import 'store.dart';
import 'text.dart';

/// The font size for message content in a plain unstyled paragraph.
const double kBaseFontSize = 14;
const double kBaseFontSize = 17;

/// The entire content of a message, aka its body.
///
Expand Down Expand Up @@ -112,7 +112,7 @@ class Paragraph extends StatelessWidget {

static const textStyle = TextStyle(
fontSize: kBaseFontSize,
height: (17 / kBaseFontSize),
height: (22 / kBaseFontSize),
);

@override
Expand Down Expand Up @@ -159,8 +159,9 @@ class Heading extends StatelessWidget {
child: _buildBlockInlineContainer(
style: TextStyle(
fontSize: kBaseFontSize * emHeight,
fontWeight: FontWeight.w600,
height: 1.4),
height: 1.4,
)
.merge(weightVariableTextStyle(context, wght: 600)),
node: node));
}
}
Expand Down Expand Up @@ -477,7 +478,7 @@ class MathBlock extends StatelessWidget {
//

Widget _buildBlockInlineContainer({
required TextStyle? style,
required TextStyle style,
required BlockInlineContainerNode node,
}) {
if (node.links == null) {
Expand All @@ -493,7 +494,7 @@ class _BlockInlineContainer extends StatefulWidget {
{required this.links, required this.style, required this.nodes});

final List<LinkNode> links;
final TextStyle? style;
final TextStyle style;
final List<InlineContentNode> nodes;

@override
Expand Down Expand Up @@ -556,7 +557,7 @@ class InlineContent extends StatelessWidget {

final GestureRecognizer? recognizer;
final Map<LinkNode, GestureRecognizer>? linkRecognizers;
final TextStyle? style;
final TextStyle style;
final List<InlineContentNode> nodes;

late final _InlineContentBuilder _builder;
Expand Down Expand Up @@ -605,13 +606,15 @@ class _InlineContentBuilder {
_recognizer = _recognizerStack!.removeLast();
}

InlineSpan _buildNodes(List<InlineContentNode> nodes, {required TextStyle? style}) {
InlineSpan _buildNodes(List<InlineContentNode> nodes, {required TextStyle style}) {
return TextSpan(
style: style,
children: nodes.map(_buildNode).toList(growable: false));
children: nodes
.map((node) => _buildNode(node, surroundingSpanStyle: style))
.toList(growable: false));
}

InlineSpan _buildNode(InlineContentNode node) {
InlineSpan _buildNode(InlineContentNode node, {required TextStyle surroundingSpanStyle}) {
if (node is TextNode) {
return TextSpan(text: node.text, recognizer: _recognizer);
} else if (node is LineBreakInlineNode) {
Expand All @@ -625,21 +628,24 @@ class _InlineContentBuilder {
} else if (node is LinkNode) {
return _buildLink(node);
} else if (node is InlineCodeNode) {
return _buildInlineCode(node);
return _buildInlineCode(node, surroundingSpanStyle: surroundingSpanStyle);
} else if (node is UserMentionNode) {
return WidgetSpan(alignment: PlaceholderAlignment.middle,
child: UserMention(node: node));
child: UserMention(node: node, surroundingSpanStyle: surroundingSpanStyle));
} else if (node is UnicodeEmojiNode) {
return TextSpan(text: node.emojiUnicode, recognizer: _recognizer);
} else if (node is ImageEmojiNode) {
return WidgetSpan(alignment: PlaceholderAlignment.middle,
child: MessageImageEmoji(node: node));
} else if (node is MathInlineNode) {
return TextSpan(style: _kInlineMathStyle,
return TextSpan(
style: surroundingSpanStyle
.merge(_kInlineMathStyle)
.apply(fontSizeFactor: _kInlineCodeFontSizeFactor),
children: [TextSpan(text: node.texSource)]);
} else if (node is GlobalTimeNode) {
return WidgetSpan(alignment: PlaceholderAlignment.middle,
child: GlobalTime(node: node));
child: GlobalTime(node: node, surroundingSpanStyle: surroundingSpanStyle));
} else if (node is UnimplementedInlineContentNode) {
return _errorUnimplemented(node);
} else {
Expand All @@ -664,7 +670,7 @@ class _InlineContentBuilder {
return result;
}

InlineSpan _buildInlineCode(InlineCodeNode node) {
InlineSpan _buildInlineCode(InlineCodeNode node, {required TextStyle surroundingSpanStyle}) {
// TODO `code` elements: border, padding -- seems hard
//
// Hard because this is an inline span, which we want to be able to break
Expand All @@ -689,7 +695,11 @@ class _InlineContentBuilder {
// TODO `code`: find equivalent of web's `unicode-bidi: embed; direction: ltr`

// Use a light gray background, instead of a border.
return _buildNodes(style: _kInlineCodeStyle, node.nodes);
return _buildNodes(
style: surroundingSpanStyle
.merge(_kInlineCodeStyle)
.apply(fontSizeFactor: _kInlineCodeFontSizeFactor),
node.nodes);

// Another fun solution -- we can in fact have a border! Like so:
// TextStyle(
Expand All @@ -709,17 +719,27 @@ class _InlineContentBuilder {
}
}

/// The [TextStyle] for inline math, excluding font-size adjustment.
///
/// Inline math should use this and also apply [_kInlineCodeFontSizeFactor]
/// to the font size of the surrounding span
/// (which might be a Paragraph, a Heading, etc.)
final _kInlineMathStyle = _kInlineCodeStyle.merge(TextStyle(
backgroundColor: const HSLColor.fromAHSL(1, 240, 0.4, 0.93).toColor()));

const _kInlineCodeFontSizeFactor = 0.825;

/// The [TextStyle] for inline code, excluding font-size adjustment.
///
/// Inline code should use this and also apply [_kInlineCodeFontSizeFactor]
/// to the font size of the surrounding span
/// (which might be a Paragraph, a Heading, etc.)
// Even though [kMonospaceTextStyle] is a variable-weight font,
// it's acceptable to skip [weightVariableTextStyle] here,
// assuming the text gets the effect of [weightVariableTextStyle]
// through inheritance, e.g., from a [DefaultTextStyle].
final _kInlineCodeStyle = kMonospaceTextStyle
.merge(const TextStyle(
backgroundColor: Color(0xffeeeeee),
fontSize: 0.825 * kBaseFontSize));
.merge(const TextStyle(backgroundColor: Color(0xffeeeeee)));

final _kCodeBlockStyle = kMonospaceTextStyle
.merge(const TextStyle(
Expand Down Expand Up @@ -750,9 +770,10 @@ final _kCodeBlockStyle = kMonospaceTextStyle
// const _kInlineCodeRightBracket = '⟩';

class UserMention extends StatelessWidget {
const UserMention({super.key, required this.node});
const UserMention({super.key, required this.node, required this.surroundingSpanStyle});

final UserMentionNode node;
final TextStyle surroundingSpanStyle;

@override
Widget build(BuildContext context) {
Expand All @@ -765,7 +786,7 @@ class UserMention extends StatelessWidget {
// One hopes an @-mention can't contain an embedded link.
// (The parser on creating a UserMentionNode has a TODO to check that.)
linkRecognizers: null,
style: Paragraph.textStyle,
style: surroundingSpanStyle,
nodes: node.nodes));
}

Expand Down Expand Up @@ -832,9 +853,10 @@ class MessageImageEmoji extends StatelessWidget {
}

class GlobalTime extends StatelessWidget {
const GlobalTime({super.key, required this.node});
const GlobalTime({super.key, required this.node, required this.surroundingSpanStyle});

final GlobalTimeNode node;
final TextStyle surroundingSpanStyle;

static final _backgroundColor = const HSLColor.fromAHSL(1, 0, 0, 0.93).toColor();
static final _borderColor = const HSLColor.fromAHSL(1, 0, 0, 0.8).toColor();
Expand All @@ -861,7 +883,7 @@ class GlobalTime extends StatelessWidget {
// Ad-hoc spacing adjustment per feedback:
// https://chat.zulip.org/#narrow/stream/101-design/topic/clock.20icons/near/1729345
const SizedBox(width: 1),
Text(text, style: Paragraph.textStyle),
Text(text, style: surroundingSpanStyle),
]))));
}
}
Expand Down Expand Up @@ -1116,8 +1138,9 @@ InlineSpan _errorUnimplemented(UnimplementedNode node) {
}
}

const errorStyle = TextStyle(fontWeight: FontWeight.bold, color: Colors.red);
const errorStyle = TextStyle(
fontSize: kBaseFontSize, fontWeight: FontWeight.bold, color: Colors.red);

final errorCodeStyle = kMonospaceTextStyle
.merge(const TextStyle(color: Colors.red))
.merge(const TextStyle(fontSize: kBaseFontSize, color: Colors.red))
.merge(weightVariableTextStyle(null)); // TODO(a11y) pass a BuildContext