Compute colorSwatchFor when subscription is null #1491
Conversation
There was a problem hiding this comment.
Thanks, LGTM except for a commit-message nit:
The base color we fall back to is still a placeholder. Ideally, we
should pick a color for the unsubscribed streams, as shown in the
Figma design linked in https://github.com/zulip/zulip-flutter/issues/188. That issue is out-of-scope for this,
though.
I was getting confused by "Ideally, we should pick a color for the unsubscribed streams", because that's exactly what this PR does, isn't it :) it picks kDefaultChannelColorSwatchBaseColor i.e. 0xffc2c2c2.
I think what's meant is that we should generate random colors (within constraints) separately for each unsubscribed channel, client-side, is that right?
(also nit: channels, not streams 🙂)
|
Thanks! Updated the PR. |
PIG208
force-pushed
the
pr-colors
branch
2 times, most recently
from
3ebe6fb to
806ea79
Compare
|
Thanks! Marking for Greg's review. |
chrisbobbe
added
integration review
Hmm. That's from 2024, and we already had the ChannelColorSwatch system (was StreamColorSwatch) in 2023. So it's not because we didn't have these swatches that we didn't use it then. Rather I think the point in that message:
was that we haven't implemented the logic to make up a different base color for each non-subscribed channel. (Which I believe draws on the same logic that's used to pick a base color for your subscription if you do subscribe.) The changes in the screenshots seem harmless, though. What's the motivation for this change — is it that in your work on #1158 you're finding you want a whole channel-color swatch for a non-subscribed channel, beyond |
|
Initially it was just the alternative solution to moving the ad-hoc color to design variables; but it is helpful for providing |
| /// For how this value is cached, see [ChannelColorSwatches.forBaseColor]. | ||
| ChannelColorSwatch colorSwatchFor(BuildContext context, Subscription subscription) { | ||
| // TODO(#188) pick different colors for unsubscribed channels |
There was a problem hiding this comment.
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 🙂.
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 (zulip#1158), giving us access to `.iconOnBarBackground` and friends for the app bar.
|
Thanks! Updated the PR. |
|
Thanks! Merging. |
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 unsubscribed streams, as shown in the Figma design linked in #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 (#1158).