From 5fcf6e060127efad38efb7d7a7982fbbaf75bdb7 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 29 Apr 2025 21:58:29 -0400 Subject: [PATCH] theme: Compute colorSwatchFor when subscription is null This removes the ad-hoc color we use for null subscriptions, which is a workaround for not having logic to generate different base colors within constraints for unsubscribed channels: https://chat.zulip.org/#narrow/channel/48-mobile/topic/All.20channels.20screen.20-.20.23F832/near/1904009 While this change does not implement that logic either, it removes the ad-hoc color in favor of a constant base color, intended for colorSwatchFor. The base color can be reused on topic-list page (#1158), giving us access to `.iconOnBarBackground` and friends for the app bar. --- lib/widgets/message_list.dart | 33 ++++++--------------------------- lib/widgets/theme.dart | 13 +++++++++++-- test/widgets/theme_test.dart | 8 ++++++++ 3 files changed, 25 insertions(+), 29 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 90a0762d34..c9e20ce87a 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -43,9 +43,6 @@ class MessageListTheme extends ThemeExtension { unreadMarker: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor(), unreadMarkerGap: Colors.white.withValues(alpha: 0.6), - - // TODO(design) this seems ad-hoc; is there a better color? - unsubscribedStreamRecipientHeaderBg: const Color(0xfff5f5f5), ); static final dark = MessageListTheme._( @@ -63,9 +60,6 @@ class MessageListTheme extends ThemeExtension { unreadMarker: const HSLColor.fromAHSL(0.75, 227, 0.78, 0.59).toColor(), unreadMarkerGap: Colors.transparent, - - // TODO(design) this is ad-hoc and untested; is there a better color? - unsubscribedStreamRecipientHeaderBg: const Color(0xff0a0a0a), ); MessageListTheme._({ @@ -76,7 +70,6 @@ class MessageListTheme extends ThemeExtension { required this.streamRecipientHeaderChevronRight, required this.unreadMarker, required this.unreadMarkerGap, - required this.unsubscribedStreamRecipientHeaderBg, }); /// The [MessageListTheme] from the context's active theme. @@ -96,7 +89,6 @@ class MessageListTheme extends ThemeExtension { final Color streamRecipientHeaderChevronRight; final Color unreadMarker; final Color unreadMarkerGap; - final Color unsubscribedStreamRecipientHeaderBg; @override MessageListTheme copyWith({ @@ -107,7 +99,6 @@ class MessageListTheme extends ThemeExtension { Color? streamRecipientHeaderChevronRight, Color? unreadMarker, Color? unreadMarkerGap, - Color? unsubscribedStreamRecipientHeaderBg, }) { return MessageListTheme._( bgMessageRegular: bgMessageRegular ?? this.bgMessageRegular, @@ -117,7 +108,6 @@ class MessageListTheme extends ThemeExtension { streamRecipientHeaderChevronRight: streamRecipientHeaderChevronRight ?? this.streamRecipientHeaderChevronRight, unreadMarker: unreadMarker ?? this.unreadMarker, unreadMarkerGap: unreadMarkerGap ?? this.unreadMarkerGap, - unsubscribedStreamRecipientHeaderBg: unsubscribedStreamRecipientHeaderBg ?? this.unsubscribedStreamRecipientHeaderBg, ); } @@ -134,7 +124,6 @@ class MessageListTheme extends ThemeExtension { streamRecipientHeaderChevronRight: Color.lerp(streamRecipientHeaderChevronRight, other.streamRecipientHeaderChevronRight, t)!, unreadMarker: Color.lerp(unreadMarker, other.unreadMarker, t)!, unreadMarkerGap: Color.lerp(unreadMarkerGap, other.unreadMarkerGap, t)!, - unsubscribedStreamRecipientHeaderBg: Color.lerp(unsubscribedStreamRecipientHeaderBg, other.unsubscribedStreamRecipientHeaderBg, t)!, ); } } @@ -225,9 +214,8 @@ class _MessageListPageState extends State implements MessageLis case ChannelNarrow(:final streamId): case TopicNarrow(:final streamId): final subscription = store.subscriptions[streamId]; - appBarBackgroundColor = subscription != null - ? colorSwatchFor(context, subscription).barBackground - : messageListTheme.unsubscribedStreamRecipientHeaderBg; + appBarBackgroundColor = + colorSwatchFor(context, subscription).barBackground; // All recipient headers will match this color; remove distracting line // (but are recipient headers even needed for topic narrows?) removeAppBarBottomBorder = true; @@ -1091,24 +1079,15 @@ class StreamMessageRecipientHeader extends StatelessWidget { // https://github.com/zulip/zulip-mobile/issues/5511 final store = PerAccountStoreWidget.of(context); final designVariables = DesignVariables.of(context); + final messageListTheme = MessageListTheme.of(context); final zulipLocalizations = ZulipLocalizations.of(context); final streamId = message.conversation.streamId; final topic = message.conversation.topic; - final messageListTheme = MessageListTheme.of(context); - - final subscription = store.subscriptions[streamId]; - final Color backgroundColor; - final Color iconColor; - if (subscription != null) { - final swatch = colorSwatchFor(context, subscription); - backgroundColor = swatch.barBackground; - iconColor = swatch.iconOnBarBackground; - } else { - backgroundColor = messageListTheme.unsubscribedStreamRecipientHeaderBg; - iconColor = designVariables.title; - } + final swatch = colorSwatchFor(context, store.subscriptions[streamId]); + final backgroundColor = swatch.barBackground; + final iconColor = swatch.iconOnBarBackground; final Widget streamWidget; if (!_containsDifferentChannels(narrow)) { diff --git a/lib/widgets/theme.dart b/lib/widgets/theme.dart index eea9677045..a533311869 100644 --- a/lib/widgets/theme.dart +++ b/lib/widgets/theme.dart @@ -554,10 +554,19 @@ class DesignVariables extends ThemeExtension { } } +// This is taken from: +// https://github.com/zulip/zulip/blob/b248e2d93/web/src/stream_data.ts#L40 +const kDefaultChannelColorSwatchBaseColor = 0xffc2c2c2; + /// The theme-appropriate [ChannelColorSwatch] based on [subscription.color]. /// +/// If [subscription] is null, [ChannelColorSwatch] will be based on +/// [kDefaultChannelColorSwatchBaseColor]. +/// /// For how this value is cached, see [ChannelColorSwatches.forBaseColor]. -ChannelColorSwatch colorSwatchFor(BuildContext context, Subscription subscription) { +// TODO(#188) pick different colors for unsubscribed channels +ChannelColorSwatch colorSwatchFor(BuildContext context, Subscription? subscription) { return DesignVariables.of(context) - .channelColorSwatches.forBaseColor(subscription.color); + .channelColorSwatches.forBaseColor( + subscription?.color ?? kDefaultChannelColorSwatchBaseColor); } diff --git a/test/widgets/theme_test.dart b/test/widgets/theme_test.dart index 8510d42b7c..678736767d 100644 --- a/test/widgets/theme_test.dart +++ b/test/widgets/theme_test.dart @@ -178,5 +178,13 @@ void main() { check(colorSwatchFor(element, subscription)) .isSameColorSwatchAs(ChannelColorSwatch.dark(baseColor)); }); + + testWidgets('fallback to default base color when no subscription', (tester) async { + await tester.pumpWidget(const TestZulipApp()); + await tester.pump(); + final element = tester.element(find.byType(Placeholder)); + check(colorSwatchFor(element, null)).isSameColorSwatchAs( + ChannelColorSwatch.light(kDefaultChannelColorSwatchBaseColor)); + }); }); }