Skip to content

theme: Add ThemeExtension for Figma design variables; add/use two more variables in AppBar #734

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.

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

Merged
merged 7 commits into from
Jun 17, 2024
3 changes: 2 additions & 1 deletion lib/widgets/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor(),
fontSize: kBaseFontSize,
letterSpacing: 0,
textBaseline: localizedTextBaseline(context),
height: (22 / kBaseFontSize),
leadingDistribution: TextLeadingDistribution.even,
decoration: TextDecoration.none,
Expand Down Expand Up @@ -345,7 +346,7 @@ class ListItemWidget extends StatelessWidget {
return Row(
mainAxisAlignment: MainAxisAlignment.start,
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: TextBaseline.alphabetic,
textBaseline: localizedTextBaseline(context),
children: [
SizedBox(
width: 20, // TODO handle long numbers in <ol>, like https://github.com/zulip/zulip/pull/25063
Expand Down
16 changes: 9 additions & 7 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -120,19 +120,21 @@ class MessageListAppBarTitle extends StatelessWidget {

final Narrow narrow;

Widget _buildStreamRow(ZulipStream? stream, String text) {
Widget _buildStreamRow(BuildContext context, {
ZulipStream? stream,
required String text,
}) {
// A null [Icon.icon] makes a blank space.
final icon = (stream != null) ? iconDataForStream(stream) : null;
return Row(
mainAxisSize: MainAxisSize.min,
// 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
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: TextBaseline.alphabetic,
crossAxisAlignment: CrossAxisAlignment.center,
children: [
Icon(size: 16, icon),
const SizedBox(width: 8),
const SizedBox(width: 4),
Flexible(child: Text(text)),
]);
}
Expand All @@ -149,13 +151,13 @@ class MessageListAppBarTitle extends StatelessWidget {
final store = PerAccountStoreWidget.of(context);
final stream = store.streams[streamId];
final streamName = stream?.name ?? '(unknown channel)';
return _buildStreamRow(stream, streamName);
return _buildStreamRow(context, stream: stream, text: streamName);

case TopicNarrow(:var streamId, :var topic):
final store = PerAccountStoreWidget.of(context);
final stream = store.streams[streamId];
final streamName = stream?.name ?? '(unknown channel)';
return _buildStreamRow(stream, "$streamName > $topic");
return _buildStreamRow(context, stream: stream, text: "$streamName > $topic");

case DmNarrow(:var otherRecipientIds):
final store = PerAccountStoreWidget.of(context);
Expand Down Expand Up @@ -899,7 +901,7 @@ class MessageWithPossibleSender extends StatelessWidget {
senderRow = Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: TextBaseline.alphabetic,
textBaseline: localizedTextBaseline(context),
children: [
Flexible(
child: GestureDetector(
Expand Down
2 changes: 1 addition & 1 deletion lib/widgets/profile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ class _ProfileDataTable extends StatelessWidget {

items.add(Row(
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: TextBaseline.alphabetic,
textBaseline: localizedTextBaseline(context),
children: [
SizedBox(width: 100,
child: Text(style: _TextStyles.customProfileFieldLabel(context),
Expand Down
28 changes: 25 additions & 3 deletions lib/widgets/text.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@ import 'package:flutter/material.dart';
/// an [AppBar]'s title, of an [ElevatedButton]'s label, and so on.
///
/// As of writing, it turns out that these styles also flow naturally into
/// most of our own widgets' text styles.
/// many of our own widgets' text styles.
/// We often see this in the child of a [Material], for example,
/// since by default [Material] applies an [AnimatedDefaultTextStyle]
/// with the [TextTheme.bodyMedium] that gets its value from here.
/// A notable exception is the base style for message content.
/// That style is self-contained and is not meant to inherit from this;
/// There are exceptions, having `inherit: false`, that are self-contained
/// and not meant to inherit from this.
/// For example, the base style for message content;
/// see [ContentTheme.textStylePlainParagraph].
///
/// Applies [kDefaultFontFamily] and [kDefaultFontFamilyFallback],
Expand Down Expand Up @@ -373,3 +374,24 @@ double proportionalLetterSpacing(
final effectiveTextScaler = textScaler ?? MediaQuery.textScalerOf(context);
return effectiveTextScaler.scale(baseFontSize) * proportion;
}

/// The most suitable [TextBaseline] for the current language.
///
/// The result is chosen based on information from [MaterialLocalizations]
/// about the language's script.
/// If [MaterialLocalizations] doesn't have that information,
/// gives [TextBaseline.alphabetic].
// Adapted from [Theme.of], which localizes text styles according to the
// locale's [ScriptCategory]. With M3 defaults, this just means varying
// [TextStyle.textBaseline] the way we do here.
TextBaseline localizedTextBaseline(BuildContext context) {
final materialLocalizations =
Localizations.of<MaterialLocalizations>(context, MaterialLocalizations);
final scriptCategory = materialLocalizations?.scriptCategory
?? ScriptCategory.englishLike;
return switch (scriptCategory) {
ScriptCategory.dense => TextBaseline.ideographic,
ScriptCategory.englishLike => TextBaseline.alphabetic,
ScriptCategory.tall => TextBaseline.alphabetic,
};
}
103 changes: 98 additions & 5 deletions lib/widgets/theme.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,45 @@ import 'content.dart';
import 'text.dart';

ThemeData zulipThemeData(BuildContext context) {
final designVariables = DesignVariables();
return ThemeData(
typography: zulipTypography(context),
extensions: [ContentTheme(context)],
appBarTheme: const AppBarTheme(
extensions: [ContentTheme(context), designVariables],
appBarTheme: AppBarTheme(
// Set these two fields to prevent a color change in [AppBar]s when
// there is something scrolled under it. If an app bar hasn't been
// given a backgroundColor directly or by theme, it uses
// ColorScheme.surfaceContainer for the scrolled-under state and
// ColorScheme.surface otherwise, and those are different colors.
scrolledUnderElevation: 0,
backgroundColor: Color(0xfff5f5f5), // `bg-top-bar` in Figma
backgroundColor: designVariables.bgTopBar,

// TODO match layout to Figma
actionsIconTheme: IconThemeData(
color: designVariables.icon,
),

titleTextStyle: TextStyle(
inherit: false,
color: designVariables.title,
fontSize: 20,
letterSpacing: 0.0,
height: (30 / 20),
textBaseline: localizedTextBaseline(context),
leadingDistribution: TextLeadingDistribution.even,
decoration: TextDecoration.none,
fontFamily: kDefaultFontFamily,
fontFamilyFallback: defaultFontFamilyFallback,
)
.merge(weightVariableTextStyle(context, wght: 600)),
titleSpacing: 4,

// TODO Figma has height 42; we should try `toolbarHeight: 42` and test
// that it looks reasonable with different system text-size settings.
// Also the back button will look too big and need adjusting.

shape: Border(bottom: BorderSide(
color: Color(0x33000000), // `border-bar` in Figma
color: designVariables.borderBar,
strokeAlign: BorderSide.strokeAlignInside, // (default restated for explicitness)
)),
),
Expand All @@ -32,7 +57,7 @@ ThemeData zulipThemeData(BuildContext context) {
colorScheme: ColorScheme.fromSeed(
seedColor: kZulipBrandColor,
),
scaffoldBackgroundColor: const Color(0xfff0f0f0), // `bg-main` in Figma
scaffoldBackgroundColor: designVariables.bgMain,
tooltipTheme: const TooltipThemeData(preferBelow: false),
);
}
Expand All @@ -42,3 +67,71 @@ ThemeData zulipThemeData(BuildContext context) {
/// This is chosen as the sRGB midpoint of the Zulip logo's gradient.
// As computed by Anders: https://github.com/zulip/zulip-mobile/pull/4467
const kZulipBrandColor = Color.fromRGBO(0x64, 0x92, 0xfe, 1);

/// Variables from the Figma design.
///
/// For how to export these from the Figma, see:
/// https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=2945-49492&t=MEb4vtp7S26nntxm-0
Comment on lines +73 to +74
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handy link!

class DesignVariables extends ThemeExtension<DesignVariables> {
DesignVariables() :
bgMain = const Color(0xfff0f0f0),
bgTopBar = const Color(0xfff5f5f5),
borderBar = const Color(0x33000000),
icon = const Color(0xff666699),
title = const Color(0xff1a1a1a);

DesignVariables._({
required this.bgMain,
required this.bgTopBar,
required this.borderBar,
required this.icon,
required this.title,
});

/// The [DesignVariables] from the context's active theme.
///
/// The [ThemeData] must include [DesignVariables] in [ThemeData.extensions].
static DesignVariables of(BuildContext context) {
final theme = Theme.of(context);
final extension = theme.extension<DesignVariables>();
assert(extension != null);
return extension!;
}

final Color bgMain;
final Color bgTopBar;
final Color borderBar;
final Color icon;
final Color title;

@override
DesignVariables copyWith({
Color? bgMain,
Color? bgTopBar,
Color? borderBar,
Color? icon,
Color? title,
}) {
return DesignVariables._(
bgMain: bgMain ?? this.bgMain,
bgTopBar: bgTopBar ?? this.bgTopBar,
borderBar: borderBar ?? this.borderBar,
icon: icon ?? this.icon,
title: title ?? this.title,
);
}

@override
DesignVariables lerp(DesignVariables? other, double t) {
if (identical(this, other)) {
return this;
}
return DesignVariables._(
bgMain: Color.lerp(bgMain, other?.bgMain, t)!,
bgTopBar: Color.lerp(bgTopBar, other?.bgTopBar, t)!,
borderBar: Color.lerp(borderBar, other?.borderBar, t)!,
icon: Color.lerp(icon, other?.icon, t)!,
title: Color.lerp(title, other?.title, t)!,
);
}
}
35 changes: 35 additions & 0 deletions test/widgets/text_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ import 'package:checks/checks.dart';
import 'package:collection/collection.dart';
import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:zulip/widgets/app.dart';
import 'package:zulip/widgets/page.dart';
import 'package:zulip/widgets/text.dart';

import '../flutter_checks.dart';
import '../model/binding.dart';

// From trying the options on an iPhone 13 Pro running iOS 16.6.1:
const kTextScaleFactors = <double>[
Expand All @@ -15,6 +18,8 @@ const kTextScaleFactors = <double>[
];

void main() {
TestZulipBinding.ensureInitialized();

group('zulipTypography', () {
Future<Typography> getZulipTypography(WidgetTester tester, {
required bool platformRequestsBold,
Expand Down Expand Up @@ -382,4 +387,34 @@ void main() {
expected: clampingTextScaler.scale(14) * 0.01);
}
});

group('localizedTextBaseline', () {
Future<void> testLocalizedTextBaseline(Locale locale, TextBaseline expected) async {
testWidgets('gives $expected for $locale', (tester) async {
addTearDown(testBinding.reset);
tester.platformDispatcher.localeTestValue = locale;
tester.platformDispatcher.localesTestValue = [locale];
addTearDown(tester.platformDispatcher.clearLocaleTestValue);
addTearDown(tester.platformDispatcher.clearLocalesTestValue);

await tester.pumpWidget(const ZulipApp());
await tester.pump();

final navigator = await ZulipApp.navigator;
navigator.push(MaterialWidgetRoute(page: Builder(builder: (context) =>
Text('123', style: TextStyle(textBaseline: localizedTextBaseline(context))))));
await tester.pumpAndSettle();

final TextStyle? style = tester.widget<Text>(find.text('123')).style;
final actualTextBaseline = style!.textBaseline!;
check(actualTextBaseline).equals(expected);
});
}

testLocalizedTextBaseline(const Locale('en'), TextBaseline.alphabetic);
testLocalizedTextBaseline(const Locale('ja'), TextBaseline.ideographic);

// "und" is a special language code meaning undefined; see [Locale]
testLocalizedTextBaseline(const Locale('und'), TextBaseline.alphabetic);
});
}
Loading