-
Notifications
You must be signed in to change notification settings - Fork 347
home: Add icons label on main navigation bar. #1879
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
329626a
22e7f3d
586c667
e6c3f5c
141b7bb
aa14c5d
4499f3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -86,35 +86,43 @@ class _HomePageState extends State<HomePage> { | |||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||||||||||||||||||
| Widget build(BuildContext context) { | ||||||||||||||||||||||||||||||||||||||||
| final zulipLocalizations = ZulipLocalizations.of(context); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const pageBodies = [ | ||||||||||||||||||||||||||||||||||||||||
| (_HomePageTab.inbox, InboxPageBody()), | ||||||||||||||||||||||||||||||||||||||||
| (_HomePageTab.channels, SubscriptionListPageBody()), | ||||||||||||||||||||||||||||||||||||||||
| // TODO(#1094): Users | ||||||||||||||||||||||||||||||||||||||||
| (_HomePageTab.directMessages, RecentDmConversationsPageBody()), | ||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| _NavigationBarButton button(_HomePageTab tab, IconData icon) { | ||||||||||||||||||||||||||||||||||||||||
| _NavigationBarButton button(_HomePageTab tab, IconData icon, String label) { | ||||||||||||||||||||||||||||||||||||||||
| return _NavigationBarButton(icon: icon, | ||||||||||||||||||||||||||||||||||||||||
| selected: _tab.value == tab, | ||||||||||||||||||||||||||||||||||||||||
| onPressed: () { | ||||||||||||||||||||||||||||||||||||||||
| _tab.value = tab; | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| label: label); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // TODO(a11y): add tooltips for these buttons | ||||||||||||||||||||||||||||||||||||||||
| final navigationBarButtons = [ | ||||||||||||||||||||||||||||||||||||||||
| button(_HomePageTab.inbox, ZulipIcons.inbox), | ||||||||||||||||||||||||||||||||||||||||
| button(_HomePageTab.inbox, ZulipIcons.inbox, | ||||||||||||||||||||||||||||||||||||||||
| zulipLocalizations.inboxPageTitle), | ||||||||||||||||||||||||||||||||||||||||
| _NavigationBarButton( icon: ZulipIcons.message_feed, | ||||||||||||||||||||||||||||||||||||||||
| selected: false, | ||||||||||||||||||||||||||||||||||||||||
| onPressed: () => Navigator.push(context, | ||||||||||||||||||||||||||||||||||||||||
| MessageListPage.buildRoute(context: context, | ||||||||||||||||||||||||||||||||||||||||
| narrow: const CombinedFeedNarrow()))), | ||||||||||||||||||||||||||||||||||||||||
| button(_HomePageTab.channels, ZulipIcons.hash_italic), | ||||||||||||||||||||||||||||||||||||||||
| narrow: const CombinedFeedNarrow())), | ||||||||||||||||||||||||||||||||||||||||
| label: zulipLocalizations.navBarFeedLabel), | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's match the name of the page, so say "Combined feed" instead of "Feed" here, using (And remove the addition of |
||||||||||||||||||||||||||||||||||||||||
| button(_HomePageTab.channels, ZulipIcons.hash_italic, | ||||||||||||||||||||||||||||||||||||||||
| zulipLocalizations.channelsPageTitle), | ||||||||||||||||||||||||||||||||||||||||
| // TODO(#1094): Users | ||||||||||||||||||||||||||||||||||||||||
| button(_HomePageTab.directMessages, ZulipIcons.two_person), | ||||||||||||||||||||||||||||||||||||||||
| button(_HomePageTab.directMessages, ZulipIcons.two_person, | ||||||||||||||||||||||||||||||||||||||||
| zulipLocalizations.navBarDmLabel), | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+120
to
+121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above—use (And remove the addition of |
||||||||||||||||||||||||||||||||||||||||
| _NavigationBarButton( icon: ZulipIcons.menu, | ||||||||||||||||||||||||||||||||||||||||
| selected: false, | ||||||||||||||||||||||||||||||||||||||||
| onPressed: () => _showMainMenu(context, tabNotifier: _tab)), | ||||||||||||||||||||||||||||||||||||||||
| onPressed: () => _showMainMenu(context, tabNotifier: _tab), | ||||||||||||||||||||||||||||||||||||||||
| label: zulipLocalizations.navBarMenuLabel), | ||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| final designVariables = DesignVariables.of(context); | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -134,17 +142,15 @@ class _HomePageState extends State<HomePage> { | |||||||||||||||||||||||||||||||||||||||
| border: Border(top: BorderSide(color: designVariables.borderBar)), | ||||||||||||||||||||||||||||||||||||||||
| color: designVariables.bgBotBar), | ||||||||||||||||||||||||||||||||||||||||
| child: SafeArea( | ||||||||||||||||||||||||||||||||||||||||
| child: SizedBox(height: 48, | ||||||||||||||||||||||||||||||||||||||||
| child: Center( | ||||||||||||||||||||||||||||||||||||||||
| child: ConstrainedBox( | ||||||||||||||||||||||||||||||||||||||||
| // TODO(design): determine a suitable max width for bottom nav bar | ||||||||||||||||||||||||||||||||||||||||
| constraints: const BoxConstraints(maxWidth: 600), | ||||||||||||||||||||||||||||||||||||||||
| child: Row( | ||||||||||||||||||||||||||||||||||||||||
| crossAxisAlignment: CrossAxisAlignment.stretch, | ||||||||||||||||||||||||||||||||||||||||
| children: [ | ||||||||||||||||||||||||||||||||||||||||
| for (final navigationBarButton in navigationBarButtons) | ||||||||||||||||||||||||||||||||||||||||
| Expanded(child: navigationBarButton), | ||||||||||||||||||||||||||||||||||||||||
| ]))))))); | ||||||||||||||||||||||||||||||||||||||||
| child: ConstrainedBox( | ||||||||||||||||||||||||||||||||||||||||
| // TODO(design): determine a suitable max width for bottom nav bar | ||||||||||||||||||||||||||||||||||||||||
| constraints: const BoxConstraints(maxWidth: 600, minHeight: 48, maxHeight: 85), | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+146
to
+147
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this explicit
I think your goal was to make sure the nav bar doesn't consume valuable space when it doesn't need to, as the text-size setting increases. We should accomplish that in a different way: I.e., don't use |
||||||||||||||||||||||||||||||||||||||||
| child: Row( | ||||||||||||||||||||||||||||||||||||||||
| crossAxisAlignment: CrossAxisAlignment.start, | ||||||||||||||||||||||||||||||||||||||||
| children: [ | ||||||||||||||||||||||||||||||||||||||||
| for (final navigationBarButton in navigationBarButtons) | ||||||||||||||||||||||||||||||||||||||||
| Expanded(child: navigationBarButton), | ||||||||||||||||||||||||||||||||||||||||
| ]))))); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
@@ -231,35 +237,50 @@ class _NavigationBarButton extends StatelessWidget { | |||||||||||||||||||||||||||||||||||||||
| required this.icon, | ||||||||||||||||||||||||||||||||||||||||
| required this.selected, | ||||||||||||||||||||||||||||||||||||||||
| required this.onPressed, | ||||||||||||||||||||||||||||||||||||||||
| required this.label, | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| final IconData icon; | ||||||||||||||||||||||||||||||||||||||||
| final bool selected; | ||||||||||||||||||||||||||||||||||||||||
| final void Function() onPressed; | ||||||||||||||||||||||||||||||||||||||||
| final String label; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||||||||||||||||||
| Widget build(BuildContext context) { | ||||||||||||||||||||||||||||||||||||||||
| final designVariables = DesignVariables.of(context); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| final iconColor = WidgetStateColor.fromMap({ | ||||||||||||||||||||||||||||||||||||||||
| WidgetState.pressed: designVariables.iconSelected, | ||||||||||||||||||||||||||||||||||||||||
| ~WidgetState.pressed: selected ? designVariables.iconSelected | ||||||||||||||||||||||||||||||||||||||||
| : designVariables.icon, | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| final color = selected ? designVariables.iconSelected : designVariables.icon; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return AnimatedScaleOnTap( | ||||||||||||||||||||||||||||||||||||||||
| scaleEnd: 0.875, | ||||||||||||||||||||||||||||||||||||||||
| duration: const Duration(milliseconds: 100), | ||||||||||||||||||||||||||||||||||||||||
| child: IconButton( | ||||||||||||||||||||||||||||||||||||||||
| icon: Icon(icon, size: 24), | ||||||||||||||||||||||||||||||||||||||||
| onPressed: onPressed, | ||||||||||||||||||||||||||||||||||||||||
| style: IconButton.styleFrom( | ||||||||||||||||||||||||||||||||||||||||
| child: Material( | ||||||||||||||||||||||||||||||||||||||||
| type: MaterialType.transparency, | ||||||||||||||||||||||||||||||||||||||||
| child: InkWell( | ||||||||||||||||||||||||||||||||||||||||
| // TODO(#417): Disable splash effects for all buttons globally. | ||||||||||||||||||||||||||||||||||||||||
| borderRadius: BorderRadius.all(Radius.circular(4)), | ||||||||||||||||||||||||||||||||||||||||
| splashFactory: NoSplash.splashFactory, | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
260
to
262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: don't separate the TODO from the line of code it is about |
||||||||||||||||||||||||||||||||||||||||
| highlightColor: designVariables.navigationButtonBg, | ||||||||||||||||||||||||||||||||||||||||
| shape: const RoundedRectangleBorder( | ||||||||||||||||||||||||||||||||||||||||
| borderRadius: BorderRadius.all(Radius.circular(4))), | ||||||||||||||||||||||||||||||||||||||||
| ).copyWith(foregroundColor: iconColor))); | ||||||||||||||||||||||||||||||||||||||||
| onTap: onPressed, | ||||||||||||||||||||||||||||||||||||||||
| child: Padding( | ||||||||||||||||||||||||||||||||||||||||
| padding: const EdgeInsets.only(bottom: 3.0), | ||||||||||||||||||||||||||||||||||||||||
| child: Column( | ||||||||||||||||||||||||||||||||||||||||
| mainAxisSize: MainAxisSize.min, | ||||||||||||||||||||||||||||||||||||||||
| children: [ | ||||||||||||||||||||||||||||||||||||||||
| SizedBox(height: 34, | ||||||||||||||||||||||||||||||||||||||||
| child: Center( | ||||||||||||||||||||||||||||||||||||||||
| child: Icon(icon, size: 24, color: color))), | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+265
to
+272
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proposed change (including the code comment, to explain to future readers):
Suggested change
For the issue the comment is about, see screenshots with and without the 3px horizontal padding:
My proposal also fixes the vertical position of the icon to match the Figma. |
||||||||||||||||||||||||||||||||||||||||
| Flexible( | ||||||||||||||||||||||||||||||||||||||||
| child: Text( | ||||||||||||||||||||||||||||||||||||||||
| label, | ||||||||||||||||||||||||||||||||||||||||
| style: TextStyle( | ||||||||||||||||||||||||||||||||||||||||
| fontSize: 12, | ||||||||||||||||||||||||||||||||||||||||
| color: color, | ||||||||||||||||||||||||||||||||||||||||
| height: 1.0,), | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+276
to
+279
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| textAlign: TextAlign.center, | ||||||||||||||||||||||||||||||||||||||||
| maxLines: 2, | ||||||||||||||||||||||||||||||||||||||||
| overflow: TextOverflow.ellipsis)), | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+281
to
+282
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we decide on max 2 lines and ellipsis overflow somewhere, or is it in the Figma? Greg pointed to an Android first-party app where the label wrapped onto three lines: #mobile-design > design comparison for nav bar label @ 💬 I think I prefer no max-lines limit at all. If a translated label is annoyingly long, I'd rather not add to the annoyance by cutting it short and adding "…". Instead I'd ask a volunteer translator if they can choose a shorter label. |
||||||||||||||||||||||||||||||||||||||||
| ]))))); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||



There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we add entries to this file, we do it in the same commit that uses them. That way it's easier to see them together, and make sure they're used as intended.