Skip to content

Compute colorSwatchFor when subscription is null #1491

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

Merged
merged 1 commit into from
May 7, 2025
Merged

Compute colorSwatchFor when subscription is null #1491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 6 additions & 27 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
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._(
Expand All @@ -63,9 +60,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
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._({
Expand All @@ -76,7 +70,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
required this.streamRecipientHeaderChevronRight,
required this.unreadMarker,
required this.unreadMarkerGap,
required this.unsubscribedStreamRecipientHeaderBg,
});

/// The [MessageListTheme] from the context's active theme.
Expand All @@ -96,7 +89,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
final Color streamRecipientHeaderChevronRight;
final Color unreadMarker;
final Color unreadMarkerGap;
final Color unsubscribedStreamRecipientHeaderBg;

@override
MessageListTheme copyWith({
Expand All @@ -107,7 +99,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
Color? streamRecipientHeaderChevronRight,
Color? unreadMarker,
Color? unreadMarkerGap,
Color? unsubscribedStreamRecipientHeaderBg,
}) {
return MessageListTheme._(
bgMessageRegular: bgMessageRegular ?? this.bgMessageRegular,
Expand All @@ -117,7 +108,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
streamRecipientHeaderChevronRight: streamRecipientHeaderChevronRight ?? this.streamRecipientHeaderChevronRight,
unreadMarker: unreadMarker ?? this.unreadMarker,
unreadMarkerGap: unreadMarkerGap ?? this.unreadMarkerGap,
unsubscribedStreamRecipientHeaderBg: unsubscribedStreamRecipientHeaderBg ?? this.unsubscribedStreamRecipientHeaderBg,
);
}

Expand All @@ -134,7 +124,6 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
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)!,
);
}
}
Expand Down Expand Up @@ -225,9 +214,8 @@ class _MessageListPageState extends State<MessageListPage> 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;
Expand Down Expand Up @@ -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)) {
Expand Down
13 changes: 11 additions & 2 deletions lib/widgets/theme.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,10 +554,19 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
}
}

// 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
Copy link
Member

Choose a reason for hiding this comment

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

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:

As discussed at #1491 (comment) , the thing we didn't have then is exactly the thing we don't have now either, the one this TODO points out 🙂.

ChannelColorSwatch colorSwatchFor(BuildContext context, Subscription? subscription) {
return DesignVariables.of(context)
.channelColorSwatches.forBaseColor(subscription.color);
.channelColorSwatches.forBaseColor(
subscription?.color ?? kDefaultChannelColorSwatchBaseColor);
}
8 changes: 8 additions & 0 deletions test/widgets/theme_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
});
});
}