Skip to content

Commit 7c9caef

Browse files
committed
theme: Compute colorSwatchFor when subscription is null
Looking into the design requirement a bit more, we initially fall back to this color because we didn't have the logic to generate channel colors: https://chat.zulip.org/#narrow/channel/48-mobile/topic/All.20channels.20screen.20-.20.23F832/near/1904009 This replaces the ad-hoc colors with generated colors based on a fallback base color. The base color we fall back to is still a placeholder. Ideally, we should pick a color even for the subscribed streams, as shown in the Figma design linked in zulip#188. That issue is out-of-scope for this, though. For now, it's sufficient to just remove the ad-hoc color on MessageListTheme. This way we don't have to reuse it in places where colors for unsubscribed streams are needed, namely the topic list.
1 parent d3a254b commit 7c9caef

File tree

3 files changed

+26
-29
lines changed

3 files changed

+26
-29
lines changed

lib/widgets/message_list.dart

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
4545
unreadMarker: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor(),
4646

4747
unreadMarkerGap: Colors.white.withValues(alpha: 0.6),
48-
49-
// TODO(design) this seems ad-hoc; is there a better color?
50-
unsubscribedStreamRecipientHeaderBg: const Color(0xfff5f5f5),
5148
);
5249

5350
static final dark = MessageListTheme._(
@@ -64,9 +61,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
6461
unreadMarker: const HSLColor.fromAHSL(0.75, 227, 0.78, 0.59).toColor(),
6562

6663
unreadMarkerGap: Colors.transparent,
67-
68-
// TODO(design) this is ad-hoc and untested; is there a better color?
69-
unsubscribedStreamRecipientHeaderBg: const Color(0xff0a0a0a),
7064
);
7165

7266
MessageListTheme._({
@@ -76,7 +70,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
7670
required this.streamRecipientHeaderChevronRight,
7771
required this.unreadMarker,
7872
required this.unreadMarkerGap,
79-
required this.unsubscribedStreamRecipientHeaderBg,
8073
});
8174

8275
/// The [MessageListTheme] from the context's active theme.
@@ -95,7 +88,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
9588
final Color streamRecipientHeaderChevronRight;
9689
final Color unreadMarker;
9790
final Color unreadMarkerGap;
98-
final Color unsubscribedStreamRecipientHeaderBg;
9991

10092
@override
10193
MessageListTheme copyWith({
@@ -105,7 +97,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
10597
Color? streamRecipientHeaderChevronRight,
10698
Color? unreadMarker,
10799
Color? unreadMarkerGap,
108-
Color? unsubscribedStreamRecipientHeaderBg,
109100
}) {
110101
return MessageListTheme._(
111102
dmRecipientHeaderBg: dmRecipientHeaderBg ?? this.dmRecipientHeaderBg,
@@ -114,7 +105,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
114105
streamRecipientHeaderChevronRight: streamRecipientHeaderChevronRight ?? this.streamRecipientHeaderChevronRight,
115106
unreadMarker: unreadMarker ?? this.unreadMarker,
116107
unreadMarkerGap: unreadMarkerGap ?? this.unreadMarkerGap,
117-
unsubscribedStreamRecipientHeaderBg: unsubscribedStreamRecipientHeaderBg ?? this.unsubscribedStreamRecipientHeaderBg,
118108
);
119109
}
120110

@@ -130,7 +120,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
130120
streamRecipientHeaderChevronRight: Color.lerp(streamRecipientHeaderChevronRight, other.streamRecipientHeaderChevronRight, t)!,
131121
unreadMarker: Color.lerp(unreadMarker, other.unreadMarker, t)!,
132122
unreadMarkerGap: Color.lerp(unreadMarkerGap, other.unreadMarkerGap, t)!,
133-
unsubscribedStreamRecipientHeaderBg: Color.lerp(unsubscribedStreamRecipientHeaderBg, other.unsubscribedStreamRecipientHeaderBg, t)!,
134123
);
135124
}
136125
}
@@ -221,9 +210,8 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
221210
case ChannelNarrow(:final streamId):
222211
case TopicNarrow(:final streamId):
223212
final subscription = store.subscriptions[streamId];
224-
appBarBackgroundColor = subscription != null
225-
? colorSwatchFor(context, subscription).barBackground
226-
: messageListTheme.unsubscribedStreamRecipientHeaderBg;
213+
appBarBackgroundColor =
214+
colorSwatchFor(context, subscription).barBackground;
227215
// All recipient headers will match this color; remove distracting line
228216
// (but are recipient headers even needed for topic narrows?)
229217
removeAppBarBottomBorder = true;
@@ -1046,24 +1034,15 @@ class StreamMessageRecipientHeader extends StatelessWidget {
10461034
// https://github.com/zulip/zulip-mobile/issues/5511
10471035
final store = PerAccountStoreWidget.of(context);
10481036
final designVariables = DesignVariables.of(context);
1037+
final messageListTheme = MessageListTheme.of(context);
10491038
final zulipLocalizations = ZulipLocalizations.of(context);
10501039

10511040
final streamId = message.conversation.streamId;
10521041
final topic = message.conversation.topic;
10531042

1054-
final messageListTheme = MessageListTheme.of(context);
1055-
1056-
final subscription = store.subscriptions[streamId];
1057-
final Color backgroundColor;
1058-
final Color iconColor;
1059-
if (subscription != null) {
1060-
final swatch = colorSwatchFor(context, subscription);
1061-
backgroundColor = swatch.barBackground;
1062-
iconColor = swatch.iconOnBarBackground;
1063-
} else {
1064-
backgroundColor = messageListTheme.unsubscribedStreamRecipientHeaderBg;
1065-
iconColor = designVariables.title;
1066-
}
1043+
final swatch = colorSwatchFor(context, store.subscriptions[streamId]);
1044+
final backgroundColor = swatch.barBackground;
1045+
final iconColor = swatch.iconOnBarBackground;
10671046

10681047
final Widget streamWidget;
10691048
if (!_containsDifferentChannels(narrow)) {

lib/widgets/theme.dart

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -561,10 +561,19 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
561561
}
562562
}
563563

564+
// This is taken from:
565+
// https://github.com/zulip/zulip/blob/b248e2d93/web/src/stream_data.ts#L40
566+
const kDefaultChannelColorSwatchBaseColor = 0xffc2c2c2;
567+
564568
/// The theme-appropriate [ChannelColorSwatch] based on [subscription.color].
565569
///
570+
/// If [subscription] is null, [ChannelColorSwatch] will be based on
571+
/// [kDefaultChannelColorSwatchBaseColor].
572+
///
566573
/// For how this value is cached, see [ChannelColorSwatches.forBaseColor].
567-
ChannelColorSwatch colorSwatchFor(BuildContext context, Subscription subscription) {
574+
// TODO(#188) pick different colors for unsubscribed channels
575+
ChannelColorSwatch colorSwatchFor(BuildContext context, Subscription? subscription) {
568576
return DesignVariables.of(context)
569-
.channelColorSwatches.forBaseColor(subscription.color);
577+
.channelColorSwatches.forBaseColor(
578+
subscription?.color ?? kDefaultChannelColorSwatchBaseColor);
570579
}

test/widgets/theme_test.dart

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'package:flutter_checks/flutter_checks.dart';
77
import 'package:flutter_test/flutter_test.dart';
88
import 'package:zulip/model/settings.dart';
99
import 'package:zulip/widgets/channel_colors.dart';
10+
import 'package:zulip/widgets/color.dart';
1011
import 'package:zulip/widgets/text.dart';
1112
import 'package:zulip/widgets/theme.dart';
1213

@@ -178,5 +179,13 @@ void main() {
178179
check(colorSwatchFor(element, subscription))
179180
.isSameColorSwatchAs(ChannelColorSwatch.dark(baseColor));
180181
});
182+
183+
testWidgets('fallback to default base color when no subscription', (tester) async {
184+
await tester.pumpWidget(const TestZulipApp());
185+
await tester.pump();
186+
final element = tester.element(find.byType(Placeholder));
187+
check(colorSwatchFor(element, null))
188+
.isSameColorSwatchAs(ChannelColorSwatch.light(Colors.white.argbInt));
189+
});
181190
});
182191
}

0 commit comments

Comments
 (0)