Use ZulipIcons.check icon for resolved topics in the UI (except topic autocomplete)#2264
Open
chrisbobbe wants to merge 7 commits into
Open
Use ZulipIcons.check icon for resolved topics in the UI (except topic autocomplete)#2264chrisbobbe wants to merge 7 commits into
ZulipIcons.check icon for resolved topics in the UI (except topic autocomplete)#2264chrisbobbe wants to merge 7 commits into
Conversation
chrisbobbe
added
the
product review
chrisbobbe
force-pushed
the
pr-checkmark-in-resolved-topics
branch
from
2487cb3 to
c47e87d
Compare
Collaborator
Author
chrisbobbe
force-pushed
the
pr-checkmark-in-resolved-topics
branch
from
c47e87d to
c820287
Compare
Member
|
Thanks @chrisbobbe! Looks like #2262 has been merged, and this has gotten some conflicts. Could you please rebase this PR to latest |
Pull out the topic-rendering part (check icon for resolved topics, topic display name, italic empty-topic placeholder) into its own topicLabelSpan function, so it can be reused at other sites that show topics.
…ader Fixes-partly: zulip#2263
Oops, I left this out of 8ac33a6; we should be using this color, specified by the Figma, instead of a Material-theme default, which Claude claims resolved to the following: - Light theme: Colors.black87 (#DD000000) - Dark theme: Colors.white This color is: - Light: Color(0xff242631) - Dark: Color(0xffdfe1e8)
…mplete Fixes-partly: zulip#2263
chrisbobbe
force-pushed
the
pr-checkmark-in-resolved-topics
branch
from
c820287 to
180947a
Compare
Collaborator
Author
|
Done, thanks for the nudge! |
rajveermalviya
approved these changes
May 4, 2026
Member
rajveermalviya
left a comment
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! All LGTM, couple of nits below.
| /// | ||
| /// Pass this to [Text.rich], which can be styled arbitrarily. | ||
| /// Pass the [fontSize] and [color] of surrounding text | ||
| /// so that the icons are sized and colored appropriately. |
Member
There was a problem hiding this comment.
nit: This helper adds only one icon.
Suggested change
| /// so that the icons are sized and colored appropriately. | |
| /// so that the icon is sized and colored appropriately. |
Comment on lines
+859
to
+860
| if (topic.unresolve().displayName != null) | ||
| TextSpan(text: topic.unresolve().displayName) |
Member
There was a problem hiding this comment.
nit: Can compute this topic.unresolve() once by assigning to a variable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is stacked atop #2262, which has a lot of commits. There are just 5 for review here:
af5a8d9 action_sheet: Use ZulipIcons.check for resolved topic in action sheet header
b9f1009 text [nfc]: Extract topicLabelSpan from channelTopicLabelSpan
03e2ed5 message_list: Use ZulipIcons.check for resolved topic in app bar title
840a867 message_list: Use ZulipIcons.check for resolved topic in recipient header
2487cb3 message_list [nfc]: Remove unused fontStyle param from recipientHeaderTextStyle
This PR leaves one site in the list at #2263 unhandled for now: topic autocomplete, just because #2225 is still in flight and I didn't want to complicate this PR further by cherry-picking that.Done; see screenshots in a separate comment below.Fixes-partly: #2263
Screenshots
cc @alya