Skip to content

icons: Add a custom icon font - #219

Merged
chrisbobbe merged 2 commits into
zulip:mainfrom
gnprice:pr-icons
Aug 1, 2023
Merged

chrisbobbe merged 2 commits into
zulip:mainfrom
gnprice:pr-icons

Conversation

@gnprice

@gnprice gnprice commented Jul 17, 2023

Copy link
Copy Markdown
Member

Fixes #200.

Also show stream privacy icons in the message-list app bar, as a demo of using the custom icons. I'll file a separate issue for doing so in the recipient headers; we'll also use them as part of implementing #156 and various other features.

@chrisbobbe chrisbobbe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Small comments below.

Comment thread lib/widgets/message_list.dart Outdated
Comment on lines 80 to 111

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

msglist: Show stream privacy icon in app bar

As discussed in the office just now, for stream and topic narrows, I notice on iOS that title part of the app bar is now start-aligned rather than centered; it seems we've lost the default behavior that should come from omitting centerTitle. It probably makes sense to keep it centered on iOS.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm yeah, thanks for catching that. Will fix.

Comment thread lib/widgets/message_list.dart Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The web app, in the heading of the views corresponding to AllMessagesNarrow and DmNarrow, these icons are used, respectively:

  • image
  • image

We could also use those.

In fact, I think the web app might have an icon for all possible message lists? Here are some others:

  • image
  • image
  • image

We don't support all message-list types that the web app does, but that might be helpful for choosing how to structure the code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Interesting, yeah.

It looks like at least some of those are FontAwesome icons, not Zulip custom icons. Here's what I see for "All messages":
<i class="fa fa-align-left" aria-hidden="true"></i>
So I think that's out of scope for this PR; I'll make a separate issue for bringing in FontAwesome icons. I might leave a blank space for it, though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

#222.

@gnprice

gnprice commented Aug 1, 2023

Copy link
Copy Markdown
Member Author

Updated! PTAL.

@chrisbobbe chrisbobbe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the review! Comments below.

Comment thread lib/widgets/message_list.dart Outdated

@chrisbobbe chrisbobbe Aug 1, 2023 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It does look a little high to me:

image

Using CrossAxisAlignment.center and removing the 4dp bottom padding is an improvement, to my eye; what do you think:

image

One thing I notice (with help from "show baselines" in the DevTools), if sticking with baseline: the icon's baseline always seems to be its bottom edge, regardless of any vertical padding. So, adjusting the 4dp bottom padding to be big or small doesn't actually change how the icon is aligned with the text. Those adjustments just change the height of the title part, which AppBar centers vertically, so the effect is just to move the icon and text up or down, together. Here's with 40dp bottom padding instead of 4:

image

With that being the case, probably it makes sense to remove that bottom padding even if staying with the baseline alignment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One thing I notice (with help from "show baselines" in the DevTools), if sticking with baseline: the icon's baseline always seems to be its bottom edge, regardless of any vertical padding. So, adjusting the 4dp bottom padding to be big or small doesn't actually change how the icon is aligned with the text.

Huh, good catch. I must have added the padding, then switched to baseline alignment, then not checked if the padding was still needed.

Using CrossAxisAlignment.center and removing the 4dp bottom padding is an improvement, to my eye; what do you think:

Right, so that's the default. Here's what that looks like for me:

image

The icon sticks out much more below the text than above it. The difference from your test is presumably that I have my system font size set to small.

If I increase my system font-size setting to the default, it's better but still seems slightly below center visually. I think it's the same as your second screenshot above. Or here it is (still with center alignment) when the text starts with "f" instead of "i":
image
That feels low — both the top and bottom of the icon are below the top and bottom respectively of the "f".

Here's that same screen with baseline alignment:
image
So I think baseline alignment works out better there than center, even at default font size.

(Going further up the scale, if I crank the font size up to large, I actually get the same results with either center or baseline alignment:
image
and they seem pretty OK.)

Comment thread lib/widgets/message_list.dart Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I'm getting some test failures, and also related-looking feedback in the debug UI:

Before After
image image

I think this would be a fix:

--- lib/widgets/message_list.dart
+++ lib/widgets/message_list.dart
@@ -88,7 +88,7 @@ class MessageListAppBarTitle extends StatelessWidget {
           padding: const EdgeInsets.only(bottom: 4),
           child: Icon(size: 16, icon)),
         const SizedBox(width: 8),
-        Text(text),
+        Expanded(child: Text(text)),
       ]);
   }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

…Oh, hmm, but that would defeat the centering we talked about in #219 (comment). Hmm.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm, good catch! It looks like those tests were introduced in #223 — so they weren't there when I initially wrote this, and I hadn't rerun flutter test since rebasing the branch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(Also I guess I rarely encounter this in manual testing because I keep my font size set to small. Even the topic in your screenshot above fits without overflow, though just barely.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks like the solution is to use Flexible instead of Expanded.

(I got there from the docs on that mainAxisSize property which I used to solve the previous issue; it had a warning effectively about how it can be defeated by an Expanded widget, but the warning also points to a way out.)

@gnprice

gnprice commented Aug 1, 2023

Copy link
Copy Markdown
Member Author

Thanks for the careful review! Updated.

@chrisbobbe
chrisbobbe merged commit 7c77e93 into zulip:main Aug 1, 2023
@chrisbobbe

Copy link
Copy Markdown
Member

Thanks! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build an asset font for custom icons

2 participants