msglist: Use icon for topic-list button in app bar #1625
Conversation
Komyyy
force-pushed
the
topics_icon
branch
2 times, most recently
from
222ad22 to
75c1a2e
Compare
|
Thanks! Could you post screenshots (light and dark mode) please? thanks! |
@chrisbobbe I uploaded screenshots on the PR message. |
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! Just small comments below.
I see you've also updated the non-English files in assets/l10n 🙂 which is helpful, I think, but not necessary. I guess since it's just turning ALL CAPS into Initial caps, it's possible to do accurately even for unfamiliar languages 🙂.
|
cc @alya to look at the screenshots |
|
Looks good to me! |
Komyyy
changed the title
chrisbobbe
added
the
integration review
|
Thanks, LGTM! Marking for Greg's review. |
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @Komyyy for taking care of this, and @chrisbobbe for the previous review! Just small comments below.
| // https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-caps#all-small-caps | ||
| fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], | ||
| ).merge(weightVariableTextStyle(context, wght: 600)))))); | ||
| return IconButton( |
There was a problem hiding this comment.
Sorry, I amended the message.
assets/l10n/app_pl.arb
Outdated
| "topicsButtonLabel": "WĄTKI", | ||
| "@topicsButtonLabel": { | ||
| "description": "Label for message list button leading to topic-list page. (Use ALL CAPS for cased alphabets: Latin, Greek, Cyrillic, etc.)" | ||
| "topicsButtonTooltip": "Watki", |
There was a problem hiding this comment.
Is this accurate? It looks like the second letter should be "ą", rather than plain "a".
There was a problem hiding this comment.
You're right, thank you: https://en.wikipedia.org/wiki/Polish_alphabet
| return IconButton( | ||
| icon: const Icon(ZulipIcons.topics), | ||
| tooltip: zulipLocalizations.topicsButtonTooltip, | ||
| onPressed: () => Navigator.push(context, | ||
| TopicListPage.buildRoute(context: context, | ||
| streamId: streamId))); |
There was a problem hiding this comment.
Nice that the code gets so much simpler!
|
Thanks! Looks good; merged. |
4 participants
Our initial implementation of #1158, in #1500, uses the word "TOPICS" for the app-bar action/button leading to the topic-list page, following the design in Figma.
We want to change it to use a "list" icon instead. This is in order to let it occupy less space, especially in languages where the translation of "TOPICS" might be long.
resolves #1532