Implement bottom tabs and main menu #1076

PIG208 · 2024-11-23T00:31:50Z

This is stacked on top of some commits cherry-picked from #1041:

action_sheet [nfc]: Use a generalized name for the cancel button
action_sheet [nfc]: Extract ActionSheetMenuItemButton

Screenshots

light	dark

Fixes: #1035

gnprice · 2024-11-25T21:23:06Z

(I turned the link to #1035 into the kind GitHub recognizes specially; that's helpful for example because it makes it appear in the issue's row on the project board). I realize the PR isn't ready to be merged as our solution to the issue, but that's expressed by its draft status; and I believe it is intended to be the PR that fixes the issue.)

gnprice

Exciting! Looking forward to getting this landed.

Comments below on the draft.

The important high-level point (of which some corollaries appear below) is: the priority with this PR is to get to something we can merge in time for the blog post about the beta.

That means a version that has all the essential functionality, looks reasonable, and meets our usual standards for the code and tests; but any aspects of the design that turn out to be tricky to implement are a lower priority than getting it merged. Because the issue is complex already, anything that's tricky and not essential should be punted until everything essential is complete.

For a concrete timeline: the target for the blog post is by Dec 12, the week after next. I think that calls for a version ready for review by the first half of next week, to give time for review, revisions, and any follow-ups we realize we need after getting to use this a bit.

gnprice · 2024-11-26T02:04:36Z

lib/widgets/home.dart

+      child: Padding(
+        padding: const EdgeInsets.only(top: 6),
+        child: Row(children: [
+          // TODO: fetch organization icon and display name from the server settings


To be sure we're on the same page: this TODO is out of scope for now, as #1037:

Suggested change

// TODO: fetch organization icon and display name from the server settings

// TODO(#1037): fetch organization icon and display name from the server settings

(or just leave out the TODO)

gnprice · 2024-11-26T02:38:59Z

lib/widgets/home.dart

+}
+
+class _OrganizationsButton extends StatelessWidget {
+  const _OrganizationsButton();


Yeah, I think this heading design may be tough to make look good before #1037. That's why in the spec in #1035 I said to leave this heading out for now and just have a "Switch account" option at the bottom of the menu.

gnprice · 2024-11-26T02:43:07Z

lib/widgets/home.dart

+  }
+}
+
+class _VersionInfo extends StatelessWidget {


This seems fine but it's also thoroughly dispensable for getting a mergeable version of this PR 🙂 So to the extent there are any open questions remaining on this widget, let's leave it out of scope.

Opened #1095 as a follow-up.

gnprice · 2024-11-26T02:47:54Z

lib/widgets/home.dart

+        }),
+        side: WidgetStateBorderSide.fromMap({
+          WidgetState.pressed: null,
+          ~WidgetState.pressed: (selected) ? borderSide : null,


nit but I see a lot of examples of it:

Suggested change

~WidgetState.pressed: (selected) ? borderSide : null,

~WidgetState.pressed: selected ? borderSide : null,

The ternary operator ?: is just an operator — it doesn't call for any of its operands to necessarily get wrapped in parens, any more than any other operator does. An operand needs parens only when it has internal structure that would otherwise get split up, or where a reader is likely to be unsure if it gets split up.

(This operator is a little special semantically in that it short-circuits. But so do the operators &&, ||, and ??, and we don't ordinarily wrap their first operands in parens either.)

gnprice · 2024-11-26T02:56:43Z

lib/widgets/home.dart

+  @override
+  // TODO: implement icon
+  IconData get icon => ZulipIcons.attach_file;
+
+  @override
+  String label(ZulipLocalizations zulipLocalizations) {
+    return 'Attach file';
+  }
+
+  @override
+  void onPressed() {
+    // TODO: implement onPressed
+  }


Probably a good structure for these is to have a subclass for each item in the menu, similar to MessageActionSheetMenuItemButton, and implement these there.

The subclasses will be a bit shorter than in the message action sheet, because onPressed will mostly just call Navigator.push with a result of SomePage.buildRoute; but between that and looking up the label on the localizations object, I think they'll be bulky enough that a subclass is more convenient than trying to cram all the details into a big list in showMenu. (There'll still be a list but it won't say much more than the names of each of the subclasses; just that and the non-static details like selected and pageContext.)

Oh and I guess there'll also be a method or getter to compute what counter to show, if any.

Maybe a good structure for that is a method like Widget? buildTrailing(BuildContext context). Then most subclasses will return a _MenuItemCounter, or null; but e.g. the "Notifications" row (in the future! it's out of scope for this PR) can return that yellow warning icon.

For the record, we decided to defer the feature to #1088.

gnprice · 2024-11-26T03:03:10Z

lib/widgets/home.dart

+  }
+}
+
+class NavigationButton extends StatelessWidget {


nit: put this just after _HomePageState, since it's closely tied to that

gnprice · 2024-11-26T03:30:57Z

lib/widgets/home.dart

+  bool handleScroll(ScrollNotification notification) {
+    animationController.paused = notification.metrics.extentBefore != 0;
+    return false;
+  }


wip; nav: Disable bottom sheet dragging when scrolled

I think the behavior before this commit is fine and actually a bit nicer. So let's leave out this complication.

I gather you're referring to this line in the Figma:

It would be nice if swipe down can close the menu, but it is important to do that only, when content is scrolled to its top position. And it isn’t just a swipe gesture but full menu dragging

But I think the thing Vlad means is just: if you've scrolled down in the content and start swiping down, it should scroll you back up in the content (just like normal for a scrollable) instead of having the menu start closing.

When I run with the version before this commit, that all works fine: if you're scrolled down, then swiping anywhere within the scrollable acts on the scrollable before doing anything to the menu as a whole. If you touch the header and drag that, then that does control the menu… but I think that's fine and actually good. After this commit, attempting to drag the header doesn't do anything if the options are scrolled, which to me feels oddly inert like something's not working.

Yeah, I agree that leaving this out is better.

It would be nice if swipe down can close the menu

I think to fully implement this (out of scope) feature, we will need to allow over scrolling the inner content (swiping down the menu items and reaching the top of the menu) to drag the bottom sheet; once you scroll pass a certain extent the menu collapses, as it does when you control it by dragging the header. I guess the menu would otherwise rebound back in position if you release it.

It makes sense to leave this out because it will be quite complicated. An implementation might have to listen to notifications like OverscrollNotification, ScrollEndNotification and replicate the drag handlers from _BottomSheetState and manipulate the animation controller.

chrisbobbe

I've read all the commits except the main commit:

nav: Add bottom tabs and main menu

which I've skimmed a bit. And I haven't done a detailed check against the Figma. But it's time for bed and I wanted to get a review in today. 🙂

Some things I noticed in manual testing:

If I tap "Switch account" from the new bottom sheet, then go back (with the back button or iOS swipe gesture), the bottom sheet is still open. Nicer to close it when you tap that button, I think. (Same for some other buttons like Mentions.)
If I do that after logging out the account I was looking at, I get an infinite loading spinner. This is what PerAccountStoreWidget.routeToRemoveOnLogout is meant to prevent. Can we use that directly, or do we need some changes to support auto-dismissing an action sheet on logout?

If I close that action sheet (by dragging down), what's supposed to happen? I get a black screen:

Is there a route (in addition to the action sheet) that didn't get removed on logout, because it didn't use PerAccountStoreWidget.routeToRemoveOnLogout? —Ah, or is this what the app looks like when there are no routes in the stack? Can we listen for that condition (no routes) and respond by basically reapplying MaterialApp.onGenerateInitialRoutes? That should give you the choose-account page. This way might be nice because it also supports cases where you logged out without requesting it in the UI, like (not yet implemented) when we learn your auth is invalid.
When you tap an account on the "Choose account" screen, I notice that a route is added to the stack, on top of the "Choose account" screen, which lets me go back to that screen. Instead, can we clear anything on the stack and replace it with just the bottom-tabs screen for the chosen account? The issue says 'there's no navigating "back" or "up" from [the screen with the bottom tab bar].'

chrisbobbe · 2024-12-03T06:16:36Z

lib/widgets/inbox.dart

@@ -14,7 +14,7 @@ import 'text.dart';
 import 'theme.dart';
 import 'unread_count_badge.dart';

-class InboxPage extends StatefulWidget {
+class InboxPage extends StatelessWidget {


nav [nfc]: Extract *PageBody widgets

This commit introduces a new pattern but doesn't use it in all the places it could. (I see at least MessageListPage is untouched.) That's fine, but the commit could be clearer that it's not focused on converting everything. Maybe:

nav [nfc]: Extract some *PageBody widgets

chrisbobbe · 2024-12-03T06:22:43Z

lib/widgets/action_sheet.dart

+      ).copyWith(backgroundColor: WidgetStateColor.fromMap({
+        WidgetState.pressed: designVariables.contextMenuCancelPressedBg,
+        ~WidgetState.pressed: designVariables.contextMenuCancelBg,
+      })),


chrisbobbe · 2024-12-03T06:32:42Z

assets/l10n/app_en.arb

@@ -15,8 +15,8 @@
  "@aboutPageTapToView": {
    "description": "Item subtitle in About Zulip page to navigate to Licenses page"
  },
-  "chooseAccountPageTitle": "Choose account",
-  "@chooseAccountPageTitle": {
+  "switchAccountPageTitle": "Switch account",


i10n: Change label from "Choose account" to "Switch account"

There are a bunch of other places where we still say "choose account":

git grep --ignore-case choose.?account

Also, what is "i10n", do you mean 'internationalization' ("intl") or localization ("l10n")? The interesting change isn't really about either. The commit implements a product choice, changing a UI string, with some boring/expected changes in assets/l10n, like we make every time we change a UI string.

We could use ui:, I guess, or (from a quick skim through this file's history):

switch-account: Rename "Choose account" page to "Switch Account"

Also, why do we make this choice? :) #1035 proposes a "Switch account" button on the new modal bottom sheet, but doesn't propose changing the choose-account page that it leads to:

For now the menu will also have a "Switch account" option at the bottom, which just leads to the choose-account page.

I think "Switch account" is appropriate for the menu button, but less appropriate for the screen with zero, one, or more accounts to choose from. "Switch account" sounds like you're in one account's context and you want to change to another. If you're seeing this page the first time you open the app, that won't be the case.

Also, what is "i10n", do you mean 'internationalization' ("intl") or localization ("l10n")?

Yeah, that's a typo. It ought to be "l10n" but I mixed up with i18n. In this PR we have been using the same strings for the page titles and the menu buttons. The button is labelled "Switch account", hence the change.

chrisbobbe · 2024-12-03T07:09:07Z

assets/l10n/app_en.arb

@@ -529,6 +529,10 @@
  "@userRoleUnknown": {
    "description": "Label for UserRole.unknown"
  },
+  "inboxButton": "Inbox",
+  "@inboxButton": {
+    "description": "Label for the menu button switching to inbox."


Nit: it doesn't switch to the inbox if you're already there :) how about:

Bottom-tab navigation button for the 'Inbox' page.

(Similarly for the other strings like this.)

Or probably better:

"inboxPageTitle": "Inbox", "@inboxPageTitle": { "description": "Title for the 'Inbox' page." },

Then we'll have one canonical name that identifies the page, for here and wherever else we need to do that. I see we don't need to do that in an app bar, because this page doesn't have an app bar. But there are probably other places we'll want to, like to structure the UI well for screen-reader software, perhaps with SemanticsProperties.namesRoute. How about adding a TODO(a11y) to find out if we're meeting users' expectations when they use Zulip with a screen reader, mentioning SemanticsProperties.namesRoute?

Updating this with the description altered to match the other existing page titles.

I guess this would also be a reason to also name the button as "Choose account" for the "Choose account" page. Both of them are temporary, and are subject to removal once we get to #1038.

The documentation for SemanticsProperties.namesRoute doesn't seem quite clear on how it can be used. I assume that we need to use Semantics with it? Because this might be a non-trivial project on its own, I feel that it might fit better as a separate issue.

I guess this would also be a reason to also name the button as "Choose account" for the "Choose account" page.

As I said in #1076 (comment), the proposal in #1035 is to label the menu button "Switch account" without changing the page's name "Choose account". For why "Choose account" makes sense to me for the page's name:

"Switch account" sounds like you're in one account's context and you want to change to another. If you're seeing this page the first time you open the app, that won't be the case.

For this menu button, I think the "Switch account" label is helpful in orienting the user (reinforcing that they're in one account's context and could move to another), so worth keeping. If we think of it as an action button that happens to act by presenting a relevant page, then I think it's fine that its label doesn't match that page's title. When the user arrives at "Choose account", it will feel natural and expected. The page's app bar (and a Semantics too?) will tell the user what page they're on, for as long as they're on the page, and we don't rely on this bottom-sheet button for that. We would if it were a bottom-tabs button, but it's not.

The documentation for SemanticsProperties.namesRoute doesn't seem quite clear on how it can be used. I assume that we need to use Semantics with it? Because this might be a non-trivial project on its own, I feel that it might fit better as a separate issue.

Yes, definitely fine to leave this for later 🙂:

How about adding a TODO(a11y) to find out if we're meeting users' expectations when they use Zulip with a screen reader, mentioning SemanticsProperties.namesRoute?

You could file an issue if you want, but I think the TODO is fine and better than nothing.

I found #535 to be relevant:

We should try out the app with iOS's VoiceOver and/or Android's TalkBack, and make sure it's possible to do the essentials to be able to use the app at all:

get between the different top-level pages (Inbox, All messages, Subscribed streams, Direct messages)

Except that the top-level pages become the tabs on the new home screen. With an app bar it might become less of a concern, but still it would be helpful to experiment with this before launch. Updated the TODO comment to refer to this issue.

chrisbobbe · 2024-12-03T07:22:53Z

lib/widgets/page.dart

-      appBar: AppBar(),
-      body: const LoadingPlaceholder(),
-    );
+    return const Scaffold(body: LoadingPlaceholder());


nav: Remove app bar from LoadingPlaceholderPage The home page when loaded does not have an app bar, so be consistent with that. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

This makes LoadingPlaceholderPage inconsistent with the other pages it's used for that do have app bars, like the message list. But we don't have a design for this loading page 🤷‍♂️ and I think I'm agnostic about whether it should have an app bar or not; sure, fine to remove.

The main reason this loading page has an app bar is to make it possible to back out of it (on iOS where you don't have a system back gesture/button) so e.g. you can go use some other account.

With the new nav, backing out wouldn't get you to ChooseAccountPage anyway because HomePage is the root of the navigation. But it'll be important to still have some way to switch accounts from the loading page.

I think that means we want two things here:

LoadingPlaceholderPage, which is used by AccountPageRouteMixin in general, should continue to have an app bar. That way if you wind up on a page that isn't HomePage, e.g. by opening a notification, and it's stuck loading, you can back out.

HomePage.buildRoute should provide some alternative loading page that's specific to HomePage. That alternative will provide a way to switch accounts, which can be ad hoc like a button in the bottom right (that opens ChooseAccountPage).

chrisbobbe · 2024-12-03T07:53:01Z

lib/widgets/home.dart

+          for (final (tab, body) in pageBodies)
+            Offstage(offstage: tab != _tab.value, child: body),
+        ]),
+      bottomNavigationBar: SafeArea(


Let's let the bottom bar's gray-colored surface extend through the bottom, left, and right insets. So, put the SafeArea as a descendant of the DecoratedBox, instead of an ancestor, I think. Currently it looks like this:

Ah, thanks for testing that out. I found out today that while my Android device has safe area, those area aren't visible at all. Maybe there's still a way to test it without an iOS device?

Maybe there's still a way to test it without an iOS device?

A good way to do that is to use the Android emulator. It has options to simulate a variety of shapes of insets.

chrisbobbe · 2024-12-03T08:10:33Z

lib/widgets/home.dart

    return Scaffold(
-      appBar: ZulipAppBar(title: const Text("Home")),


There's a top bar in the Figma:

Ahh but I see #1035 says "There's no top bar" in the issue description. Fair enough 🤷‍♂️ but let's mention it in the commit message's list of differences from the Figma. Also, now we don't have a spec for what goes in the top inset; here's this revision:

Not scrolled Scrolled a bit

The top inset is filled with the same color we use under the content in the y-direction when the list is short. I don't love that; it's like the content just abruptly disappears, instead of sliding under some different surface (what used to be the app bar). I don't know what's best here, perhaps just leave a TODO(design) for how to fill the top inset on these pages.

Yeah, that will be a separate redesign project. I recalled that we should have an issue for the top bar other than #1039, but I couldn't find it.

Let's have an app bar at the top after all. (It'll be pretty plain — just a title, I think.)

I'm not sure what my thinking was when I said in the issue there's no top bar. But I see that's a bit of a challenge what should happen on scroll, if there's no app bar; and I just went and looked through several apps on my phone that have a bottom nav bar, and all of them have an app bar on those screens. In a couple of cases the app bar goes away as you scroll (making more room for the scrolling content), but that's trickier; let's skip that at least for launch, and have the app bar remain fixed at the top on scroll.

I went to go update #1035 with this change, and rereading the context of that sentence I realized what happened: what I really meant was that there's no top tab bar. I probably didn't consider at that moment the question of whether there should or shouldn't be a plain title-bearing app bar instead.

Anyway, fixed there.

gnprice

To help accelerate things for this PR, here's a review of some commits that I think we can merge early. @PIG208 would you send a separate PR with these, after revision for the nit below and applicable comments by @chrisbobbe above?

48671b2 nav [nfc]: Extract *PageBody widgets
70777ba nav [nfc]: Move HomePage to a new home
8488162 action_sheet [nfc]: Centralize .withValues variants of contextMenuCancelBg
6d9037b nav: Add icons from Figma

In particular I think some of those are likely to be conflict-prone — icons changes always are, and theme changes often seem to be — so it's helpful to have them merged early.

gnprice · 2024-12-04T00:04:56Z

lib/widgets/icons.dart

+  /// The Zulip custom icon "contacts".
+  static const IconData contacts = IconData(0xf109, fontFamily: "Zulip Icons");


nit:

nav: Add icons from Figma The icons are taken from https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=544-22131&node-type=canvas&t=f4vYPq2CZUpFuf7V-0.

Write instead:

icons: Add icons for bottom nav bar The icons are taken from Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=544-22131&node-type=canvas&t=f4vYPq2CZUpFuf7V-0

Avoiding . there avoids ambiguity about what the URL is.

The summary line can be more specific than "nav" about what the icons are; there are other icons involved in nav. And our usual prefix for editing icons is "icons:".

gnprice

Further in the interest of accelerating things for this PR, here's an initial review from me of the main commit:
6e8de91 nav: Add bottom tabs and main menu

I skipped points that are already discussed in Chris's review above.

gnprice · 2024-12-04T00:19:53Z

lib/widgets/home.dart

+          child: Align(
+            // This is necessary to limit the width of the row of icons,
+            // because [Align] allows the child to be smaller than itself.
+            heightFactor: 1,


I don't follow this comment or what this Align widget is doing. A height factor is necessary to limit the width of the row?

Why Align and not Center?

Moved this comment above and added another one explaining the heightFactor. Also changed it to use Center instead.

Much clearer, thanks!

gnprice · 2024-12-04T00:23:04Z

lib/widgets/home.dart

+            // because [Align] allows the child to be smaller than itself.
+            heightFactor: 1,
+            child: ConstrainedBox(
+              // TODO(design): determine a suitable max width


nit:

Suggested change

// TODO(design): determine a suitable max width

// TODO(design): determine a suitable max width for bottom nav bar

That way it's easier to tell what this is about when grepping for TODO, or for TODO.design.

gnprice · 2024-12-04T00:30:02Z

lib/widgets/home.dart

+  }
+}
+
+class NavigationButton extends StatelessWidget {


nit:

Suggested change

class NavigationButton extends StatelessWidget {

class _NavigationBarButton extends StatelessWidget {

We have many buttons that are used for navigation; these are just those that appear in this particular context.

gnprice · 2024-12-04T00:36:31Z

lib/widgets/home.dart

+          child: Column(
+            crossAxisAlignment: CrossAxisAlignment.stretch,
+            mainAxisSize: MainAxisSize.max,


nit:

Suggested change

child: Column(

crossAxisAlignment: CrossAxisAlignment.stretch,

mainAxisSize: MainAxisSize.max,

child: Column(

crossAxisAlignment: CrossAxisAlignment.stretch,

This is the default and we normally don't repeat it.

gnprice · 2024-12-04T00:38:07Z

lib/widgets/home.dart

+                  child: ConstrainedBox(
+                    constraints: const BoxConstraints(minHeight: 44),
+                    child: const ActionSheetCancelButton()))),


Seems like this min-height should be the button's business.

Can it really be shorter than this anyway? I'd think TextButton (which this button uses) would take care of that.

With a smaller font, the button can get a bit shorter than that:

Small font (compare in different tabs)

w/ minHeight w/o minHeight

(Are the labels switched on those screenshots? The one labeled "w/ minHeight" has the button shorter than the other does.)

Following up on this point:

I'd think TextButton (which this button uses) would take care of that.

by looking into TextButton, it looks like this is controlled by ButtonStyle.minimumSize. The default for which in TextButton.defaultStyleOf is:

@override MaterialStateProperty<Size>? get minimumSize => const MaterialStatePropertyAll<Size>(Size(64.0, 40.0));

That's a minimum of 64px wide but only 40px high. So TextButton does indeed take care of this need — but it sets a slightly smaller minimum.

What if we set TextButton.style to provide a minimumSize property there?

What if we set TextButton.style to provide a minimumSize property there?

Yeah, that should be cleaner.

Updated screenshots

font size before after

extra small

normal /

gnprice · 2024-12-04T00:39:14Z

lib/widgets/home.dart

+                  child: Column(
+                    mainAxisAlignment: MainAxisAlignment.start,


similarly:

Suggested change

child: Column(

mainAxisAlignment: MainAxisAlignment.start,

child: Column(

gnprice · 2024-12-04T00:41:19Z

lib/widgets/home.dart

+  }
+}
+
+abstract class _NavigationBarMenuButton extends _MenuButton {


This calls for a few words of dartdoc — from the name, it sounds like "a button for the navigation-bar menu", which is confusing because that's what _MenuButton is.

gnprice · 2024-12-04T00:46:21Z

lib/widgets/home.dart

+            Expanded(child: Text(label(zulipLocalizations),
+              textAlign: TextAlign.start,


Surely TextAlign.start is the default, right? So we shouldn't need to repeat it:

Suggested change

Expanded(child: Text(label(zulipLocalizations),

textAlign: TextAlign.start,

Expanded(child: Text(label(zulipLocalizations),

gnprice · 2024-12-04T01:08:13Z

test/widgets/home_test.dart

+      await tapOpenMenu(tester);
+      checkIconSelected(tester, inboxMenuIconFinder);
+      checkIconNotSelected(tester, channelsMenuIconFinder);
+      check(find.byType(InboxPageBody)).findsOne();


Suggested change

check(find.byType(InboxPageBody)).findsOne();

check(find.byType(InboxPageBody)).findsOne();

check(find.byType(SubscriptionListPageBody)).findsNothing();

and similarly check(find.byType(InboxPageBody)).findsNothing(); below. That helps the reader be sure that something really changed as a result of the tap — that it's not just that both finders already would find something and continue to do so (e.g. because we got something wrong in the interaction of the finders with the Offstage nature of the pages).

PIG208 · 2024-12-04T04:27:23Z

This addresses most of the comment except
#1076 (comment) (special loading page) and #1076 (review) (navigation change) and #1076 (comment) (app bar). All of these mentioned features are included as WIP for now. Will get back to this tomorrow.

chrisbobbe · 2024-12-04T19:05:52Z

The prep PR #1099 was merged, so this can be updated for that. (Also I see CI is failing.)

PIG208 · 2024-12-04T19:06:26Z

Yes. There are some wip changes to be finished before this is ready for review again.

PIG208 · 2024-12-05T07:25:28Z

Thanks for the reviews @chrisbobbe @gnprice! The PR is ready again.

Some UI changes:

HomePage's loading screen

button normal	button pressed

The button uses the default styling of TextButton for simplicity.

HomePage's app bar

This also adds a 16px titleSpacing conditioned on whether a back button is there

Now we ensure that the HomePage is always at the root level, by removing all the existing routes when pushing it onto the navigator stack.

There are also some minor changes like popping the bottom sheet for all menu buttons.

gnprice

Thanks for the revision!

It's late, so for tonight here's just a review of the points that were in my previous review #1076 (review) . All look good now except for the couple of items below.

gnprice · 2024-12-05T08:02:30Z

lib/widgets/home.dart

+  }
+}
+
+class _NavigationButton extends StatelessWidget {


bump #1076 (comment)

I see. Missed the Bar part. Fixed that and updated comment/string references to this name to match.

gnprice · 2024-12-05T08:06:41Z

lib/widgets/home.dart

+                  child: ConstrainedBox(
+                    constraints: const BoxConstraints(minHeight: 44),
+                    child: const ActionSheetCancelButton()))),


(Are the labels switched on those screenshots? The one labeled "w/ minHeight" has the button shorter than the other does.)

gnprice · 2024-12-05T08:16:14Z

lib/widgets/home.dart

+                  child: ConstrainedBox(
+                    constraints: const BoxConstraints(minHeight: 44),
+                    child: const ActionSheetCancelButton()))),


Following up on this point:

I'd think TextButton (which this button uses) would take care of that.

by looking into TextButton, it looks like this is controlled by ButtonStyle.minimumSize. The default for which in TextButton.defaultStyleOf is:

@override MaterialStateProperty<Size>? get minimumSize => const MaterialStatePropertyAll<Size>(Size(64.0, 40.0));

That's a minimum of 64px wide but only 40px high. So TextButton does indeed take care of this need — but it sets a slightly smaller minimum.

What if we set TextButton.style to provide a minimumSize property there?

gnprice

Thanks for the revision! I played with this UI and am happy to see that the behavior seems to be basically all working.

Partial review below, including one behavior issue in the switch-account button being potentially inaccessible. Posting the partial review now in the interest of pipelining work on this PR.

Another thing I noticed in the UI is that the menu takes up the full height of the screen even when its contents are much shorter. Is that something Vlad specifically addressed in his design? My instinct would be to let the menu be the height of its contents when there's room, and fill the screen only when it needs to.

gnprice · 2024-12-05T19:57:17Z

lib/widgets/page.dart

@@ -67,6 +68,7 @@ class MaterialAccountPageRoute<T extends Object?> extends MaterialPageRoute<T> w
  MaterialAccountPageRoute({
    int? accountId,
    BuildContext? context,
+    this.loadingPlaceholderPage,


nit: add this option in a prep commit, to simplify the main commit a bit

gnprice · 2024-12-05T21:29:03Z

test/widgets/app_test.dart


-    Future<List<Route<dynamic>>> initialRoutes(WidgetTester tester) async {


nit:

app test [nfc]: Convert initialRoute into a generic helper

name misspelled in summary line

gnprice · 2024-12-05T21:36:29Z

test/widgets/app_test.dart

+      ]);
+    });
+
+    testWidgets('choosing an account clears the navigator stack', (tester) async {


This doesn't seem to belong in a group for "ZulipApp initial navigation". It's really about ChooseAccountPage.

I think the existing tests in this group can stay as they are (except the update to expectations), and this one can do its own setup of a nav observer. It doesn't really have the same needs in a nav observer as the other tests in this group do, and combining their needs in one helper makes the other tests more complicated to understand.

Moving this to the "ChooseAccountPage" group.

gnprice · 2024-12-05T21:53:41Z

test/widgets/login_test.dart

        await prepare(tester, serverSettings);
+        checkNotLoggedInStartingRoutes();
+        pushedRoutes.clear();


This no longer checks as much as it did before — it won't notice if there were excess routes pushed on top of the expected ones.

I think the previous take/check-empty structure was good. You can move that verbatim out of prepare to its callers.

gnprice · 2024-12-05T21:57:32Z

test/widgets/login_test.dart

+          (it) => it.isA<MaterialAccountWidgetRoute>()
+            ..accountId.equals(eg.selfAccount.id)
+            ..page.isA<HomePage>(),
+          (it) => it.isA<MaterialWidgetRoute>()
+            ..page.isA<LoginPage>(),


nit: this can be a lot terser, like the existing check:

Suggested change

(it) => it.isA<MaterialAccountWidgetRoute>()

..accountId.equals(eg.selfAccount.id)

..page.isA<HomePage>(),

(it) => it.isA<MaterialWidgetRoute>()

..page.isA<LoginPage>(),

(it) => it.isA<WidgetRoute>().page.isA<HomePage>(),

(it) => it.isA<WidgetRoute>().page.isA<LoginPage>(),

The one substantive change there is that it doesn't check the account ID on the HomePage. That's fine because that HomePage isn't part of what this test is about, but rather part of the background to it.

gnprice · 2024-12-05T21:58:51Z

test/widgets/login_test.dart

+        check(poppedRoutes).deepEquals(<Condition<Object?>>[
+          (it) => it.isA<MaterialWidgetRoute>()
+            ..page.isA<LoginPage>(),
+          (it) => it.isA<MaterialAccountWidgetRoute>()
+            ..accountId.equals(eg.selfAccount.id)
+            ..page.isA<HomePage>(),
+        ]);


This is just popping all the routes that were on the stack, right? So I think just counting the pops covers it:

Suggested change

check(poppedRoutes).deepEquals(<Condition<Object?>>[

(it) => it.isA<MaterialWidgetRoute>()

..page.isA<LoginPage>(),

(it) => it.isA<MaterialAccountWidgetRoute>()

..accountId.equals(eg.selfAccount.id)

..page.isA<HomePage>(),

]);

check(poppedRoutes).length.equals(2);

gnprice · 2024-12-05T22:00:29Z

test/widgets/login_test.dart

+        pushedRoutes.clear();
+
+        await login(tester, eg.otherAccount);
+        final loggedInAccount = testBinding.globalStore.accounts.singleWhere(


nit: "logged-in account" is a confusing name here, because both accounts are logged in.

Suggested change

final loggedInAccount = testBinding.globalStore.accounts.singleWhere(

final newAccount = testBinding.globalStore.accounts.singleWhere(

gnprice · 2024-12-05T22:07:41Z

lib/widgets/home.dart

+      body: Column(
+        crossAxisAlignment: CrossAxisAlignment.end,
+        children: [
+          const Expanded(child: LoadingPlaceholder()),
+          TextButton(


This needs to use SafeArea to stay out of the insets. Here's how it looks on my phone:

Probably cleanest is to make this a floating action button:

return Scaffold( body: const LoadingPlaceholder(), floatingActionButton: TextButton( onPressed: () => Navigator.push(context, MaterialWidgetRoute(page: const ChooseAccountPage())), child: Text(zulipLocalizations.chooseAccountPageTitle)), );

That takes care of applying SafeArea; and it also means the layout of LoadingPlaceholder and centering of the circular progress indicator doesn't get distorted by the presence of the button.

In this revision I also added the AppBar back because HomePage has gained one in the previous update.

gnprice

OK, and here is the rest of a complete review! Eager to get this merged.

I think the one user-visible item is the layout for the "Switch account" button. So that's the most important to fix. Then there's a bit of a long list of comments, but I think the bulk of them will be quick to take care of.

gnprice · 2024-12-10T23:57:05Z

lib/widgets/theme.dart

+    seedColor: kZulipBrandColor);
  switch (brightness) {


nit: set off this stanza with blank lines (here and at top of comment):

Suggested change

seedColor: kZulipBrandColor);

switch (brightness) {

seedColor: kZulipBrandColor);

switch (brightness) {

Similar to #1076 (comment).

Potentially would have been good in the existing code too; but in a list, like where the existing version of this was, it's already clear where one item ends and the next begins, so it's natural to take a comment just before an item as referring to that one item.

gnprice · 2024-12-10T23:59:03Z

test/widgets/actions_test.dart

+      // TODO(#737): switch to a realistic setup
+      final account1Route = MaterialAccountWidgetRoute(
+        accountId: account1.id, page: const InboxPageBody());
+      final account2Route = MaterialAccountWidgetRoute(
+        accountId: account2.id, page: const InboxPageBody());


Suggested change

// TODO(#737): switch to a realistic setup

final account1Route = MaterialAccountWidgetRoute(

accountId: account1.id, page: const InboxPageBody());

final account2Route = MaterialAccountWidgetRoute(

accountId: account2.id, page: const InboxPageBody());

// TODO(#737): switch to a realistic setup:

// https://github.com/zulip/zulip-flutter/pull/1076#discussion_r1874124363

final account1Route = MaterialAccountWidgetRoute(

accountId: account1.id, page: const InboxPageBody());

final account2Route = MaterialAccountWidgetRoute(

accountId: account2.id, page: const InboxPageBody());

gnprice · 2024-12-11T00:00:18Z

lib/widgets/app.dart

+    final hasBackButton = ModalRoute.of(context)?.impliesAppBarDismissal ?? false;
+    return MenuButtonTheme(


bump #1076 (comment) :

Suggested change

final hasBackButton = ModalRoute.of(context)?.impliesAppBarDismissal ?? false;

return MenuButtonTheme(

final hasBackButton = ModalRoute.of(context)?.impliesAppBarDismissal ?? false;

return MenuButtonTheme(

As I mentioned, the point is to make clear what code the comment is describing. So the blank line marking the end of that stretch of code is key for that.

gnprice · 2024-12-11T00:03:01Z

lib/widgets/home.dart

 import 'recent_dm_conversations.dart';
 import 'store.dart';
 import 'subscription_list.dart';
 import 'text.dart';
+import 'theme.dart';
+
+enum HomePageTab {


nit:

Suggested change

enum HomePageTab {

enum _HomePageTab {

gnprice · 2024-12-11T00:09:49Z

lib/widgets/home.dart

+}
+
+class _HomePageState extends State<HomePage> {
+  late final ValueNotifier<HomePageTab> _tab = ValueNotifier(HomePageTab.inbox);


nit:

Suggested change

late final ValueNotifier<HomePageTab> _tab = ValueNotifier(HomePageTab.inbox);

late final _tab = ValueNotifier(HomePageTab.inbox);

gnprice · 2024-12-11T01:25:26Z

test/widgets/home_test.dart

+    testWidgets('while loading, go back from ChooseAccountPage', (tester) async {
+      testBinding.globalStore.loadPerAccountDuration = loadPerAccountDuration;
+      await prepare(tester);
+      await tester.pump(kTryAnotherAccountWaitPeriod);
+      await tapChooseAccount(tester);
+
+      await tester.tap(find.byType(BackButton));
+      await tester.pump(Duration.zero); // tap the button
+      await tester.pump(const Duration(milliseconds: 350)); // wait for pop animation
+      checkOnLoadingPage();
+
+      await tester.pump(loadPerAccountDuration);
+      checkOnHomePage(tester, expectedAccount: eg.selfAccount);
+    });


I like these nice clear tests! The various helper functions really contribute to being able to tell these different stories cleanly.

gnprice · 2024-12-11T01:26:56Z

test/widgets/home_test.dart

+      // The second loadPerAccount finished.
+      await tester.pump(loadPerAccountDuration);
+      checkOnHomePage(tester, expectedAccount: eg.otherAccount);


nit: comment that says something is true, in the present tense, should go at a point where it's become true

Suggested change

// The second loadPerAccount finished.

await tester.pump(loadPerAccountDuration);

checkOnHomePage(tester, expectedAccount: eg.otherAccount);

await tester.pump(loadPerAccountDuration);

// The second loadPerAccount finished.

checkOnHomePage(tester, expectedAccount: eg.otherAccount);

gnprice · 2024-12-11T01:28:15Z

test/widgets/home_test.dart

+      // The second loadPerAccount is still pending.
+      await tester.pump(loadPerAccountDuration);
+      checkOnLoadingPage();


nit: similarly:

Suggested change

// The second loadPerAccount is still pending.

await tester.pump(loadPerAccountDuration);

checkOnLoadingPage();

await tester.pump(loadPerAccountDuration);

// The second loadPerAccount is still pending.

checkOnLoadingPage();

Here the statement in the comment was already true before the pump, but it wasn't interesting — of course it's still pending, only a few seconds have passed. It's after the await that that fact becomes interesting (which corresponds to the fact that that's the point where you're checking it).

gnprice · 2024-12-11T01:28:36Z

test/widgets/home_test.dart

+      checkOnHomePage(tester, expectedAccount: eg.otherAccount);
+    });
+
+    testWidgets('while loading, choosing an account disallow going back', (tester) async {


nit:

Suggested change

testWidgets('while loading, choosing an account disallow going back', (tester) async {

testWidgets('while loading, choosing an account disallows going back', (tester) async {

gnprice · 2024-12-11T01:40:54Z

test/widgets/recent_dm_conversations_test.dart

+  // Switch to direct messages tab.
+  await tester.tap(find.descendant(
+    of: find.byType(Center),
+    matching: find.byIcon(ZulipIcons.user)));


Hmm, the use of Center is awkward — it's rather an implementation detail, both the fact that the desired copy of the icon does have a Center ancestor and the fact that the undesired copies don't.

I don't have a better idea to suggest, though, so I guess just leave it. No need for a TODO, even; really it just becomes actionable if the test breaks (because of some innocent change to either the nav bar, or how DMs appear in the inbox, or something else) and we have to find a different solution.

I feel like the API I'd want here is to be able to say "tap the such-and-such icon, specifically the one that's in the bottom part of the screen". But I don't see an easy way to do that with the existing Finder API.

Yeah, I was a bit reluctant when writing this, but some alternatives would be customizing the initial view, adding a debug feature, or anything that only gets used in tests like this, which are not really realistic/useful. Leaving it seems fine to me.

There is tapAt that we can use, but with that we still need to know roughly where the buttons are. It's probably doable but requires a few more steps of setup.

This is an initial implementation because some features were considered out-of-scope as of now for zulip#1035. Compared to the Figma, we swapped the order of _ChannelsButton and _DirectMessagesButton in the menu so they match their order on the navigation bar. See: https://chat.zulip.org/#narrow/channel/48-mobile/topic/Buttons.20on.20the.20bottom.20tabs.20and.20main.20menu We also added _CombinedFeedButton, using the same icon we have for "Combined feed" on the web app's topleft sidebar, added a "Switch account" button, and renamed "Streams" to "Channels". For now, we do not aim to fully implement the app bar redesign; we keep a simple one in this implementation. We maintain a 16px padding for the title text on the app bar for both ChooseAccountPage (when it is at the root navigation level) and HomePage (which is expected to be at the root level all the time). We allow user to navigate to the choose-account page from the loading screen after a certain timeout. This is helpful when loading the account data takes a while, and that the user, for example, wants to try another account. See: zulip#1076 (comment) Signed-off-by: Zixuan James Li <zixuan@zulip.com>

gnprice

Thanks for the swift revision! Just two nits now.

gnprice · 2024-12-11T03:17:35Z

test/widgets/actions_test.dart

+      // TODO(#737): switch to a realistic setup:
+      final account1Route = MaterialAccountWidgetRoute(


bump #1076 (comment) 🙂

Ohh! I was wondering why there was only a colon added (which made some sense to me anyway).

gnprice · 2024-12-11T03:19:30Z

lib/widgets/home.dart

+    return Icon(icon, size: _iconSize, color: selected ? designVariables.iconSelected
+                                                : designVariables.icon);


nit: misaligned

(probably just move color to second line)

gnprice · 2024-12-11T03:24:11Z

lib/widgets/home.dart

+  Widget buildLeading(BuildContext context) {
+    final store = PerAccountStoreWidget.of(context);
+    return Avatar(
+      userId: store.selfUserId, size: _MenuButton._iconSize, borderRadius: 4);


Ah, bonus factoring-out! 🙂

This is an initial implementation because some features were considered out-of-scope as of now for zulip#1035. Compared to the Figma, we swapped the order of _ChannelsButton and _DirectMessagesButton in the menu so they match their order on the navigation bar. See: https://chat.zulip.org/#narrow/channel/48-mobile/topic/Buttons.20on.20the.20bottom.20tabs.20and.20main.20menu We also added _CombinedFeedButton, using the same icon we have for "Combined feed" on the web app's topleft sidebar, added a "Switch account" button, and renamed "Streams" to "Channels". For now, we do not aim to fully implement the app bar redesign; we keep a simple one in this implementation. We maintain a 16px padding for the title text on the app bar for both ChooseAccountPage (when it is at the root navigation level) and HomePage (which is expected to be at the root level all the time). We allow user to navigate to the choose-account page from the loading screen after a certain timeout. This is helpful when loading the account data takes a while, and that the user, for example, wants to try another account. See: zulip#1076 (comment) Signed-off-by: Zixuan James Li <zixuan@zulip.com>

gnprice · 2024-12-11T23:04:40Z

@PIG208 would you rebase? Looks like the last commit has a non-local conflict with #1041 (should have merged this one before that one, I guess, oops).

gnprice · 2024-12-11T23:04:47Z

Otherwise all LGTM!

With a small font size, the button can be shorter than it is specified in the Figma design. See: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3479-26262&node-type=symbol&t=hGZtzX2QTLse81un-0 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

This allows us to use `prepare` for testing the login page when the user is logged in. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

This preps for adding a special loading page for the home page. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

This applies to all the `ElevatedButton`'s, which are exclusively used in `HomePage`, `LoginPage`, `ChooseAccountPage`, and `AddAccountPage, to fix their color in dark mode. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

The colors are intended to match what we use for `ElevatedButton` (see `zulipThemeData`). Signed-off-by: Zixuan James Li <zixuan@zulip.com>

This is an initial implementation because some features were considered out-of-scope as of now for zulip#1035. Compared to the Figma, we swapped the order of _ChannelsButton and _DirectMessagesButton in the menu so they match their order on the navigation bar. See: https://chat.zulip.org/#narrow/channel/48-mobile/topic/Buttons.20on.20the.20bottom.20tabs.20and.20main.20menu We also added _CombinedFeedButton, using the same icon we have for "Combined feed" on the web app's topleft sidebar, added a "Switch account" button, and renamed "Streams" to "Channels". For now, we do not aim to fully implement the app bar redesign; we keep a simple one in this implementation. We maintain a 16px padding for the title text on the app bar for both ChooseAccountPage (when it is at the root navigation level) and HomePage (which is expected to be at the root level all the time). We allow user to navigate to the choose-account page from the loading screen after a certain timeout. This is helpful when loading the account data takes a while, and that the user, for example, wants to try another account. See: zulip#1076 (comment) Signed-off-by: Zixuan James Li <zixuan@zulip.com>

…Page Fixes: zulip#1035 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

PIG208 · 2024-12-11T23:10:16Z

Rebased and fixed the conflict with $ git rebase -x 'tools/check l10n --fix'. Additionally addressed the issue that InboxPage no longer exists for a test from #1041.

gnprice · 2024-12-11T23:15:45Z

Thanks!

CI has reached the android suite (which is the last one because it's slow), and it looks like the others pass. That one shouldn't be affected by these changes, so merging.

This is an initial implementation because some features were considered out-of-scope as of now for zulip#1035. Compared to the Figma, we swapped the order of _ChannelsButton and _DirectMessagesButton in the menu so they match their order on the navigation bar. See: https://chat.zulip.org/#narrow/channel/48-mobile/topic/Buttons.20on.20the.20bottom.20tabs.20and.20main.20menu We also added _CombinedFeedButton, using the same icon we have for "Combined feed" on the web app's topleft sidebar, added a "Switch account" button, and renamed "Streams" to "Channels". For now, we do not aim to fully implement the app bar redesign; we keep a simple one in this implementation. We maintain a 16px padding for the title text on the app bar for both ChooseAccountPage (when it is at the root navigation level) and HomePage (which is expected to be at the root level all the time). We allow user to navigate to the choose-account page from the loading screen after a certain timeout. This is helpful when loading the account data takes a while, and that the user, for example, wants to try another account. See: zulip#1076 (comment) Signed-off-by: Zixuan James Li <zixuan@zulip.com>

PIG208 changed the title ~~action_sheet [nfc]: Extract ActionSheetMenuItemButton~~ WIP Implement bottom tabs and main menu Nov 23, 2024

PIG208 force-pushed the pr-nav branch 2 times, most recently from eb8c896 to 5c927c1 Compare November 23, 2024 00:44

gnprice reviewed Nov 26, 2024

View reviewed changes

PIG208 force-pushed the pr-nav branch 2 times, most recently from f8a9e5a to 15503a5 Compare December 3, 2024 00:40

PIG208 changed the title ~~WIP Implement bottom tabs and main menu~~ Implement bottom tabs and main menu Dec 3, 2024

PIG208 marked this pull request as ready for review December 3, 2024 00:41

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Dec 3, 2024

PIG208 assigned chrisbobbe Dec 3, 2024

PIG208 requested a review from chrisbobbe December 3, 2024 00:41

PIG208 force-pushed the pr-nav branch from 15503a5 to 733daf6 Compare December 3, 2024 00:44

chrisbobbe reviewed Dec 3, 2024

View reviewed changes

gnprice reviewed Dec 4, 2024

View reviewed changes

PIG208 mentioned this pull request Dec 4, 2024

Prep for bottom tabs and main menu #1099

Merged

PIG208 force-pushed the pr-nav branch from 733daf6 to 23bdc0c Compare December 4, 2024 04:18

gnprice mentioned this pull request Dec 5, 2024

Support adding arbitrary reactions #1103

Merged

PIG208 force-pushed the pr-nav branch from 23bdc0c to 87efdf0 Compare December 5, 2024 07:14

PIG208 requested a review from chrisbobbe December 5, 2024 07:14

gnprice reviewed Dec 5, 2024

View reviewed changes

PIG208 force-pushed the pr-nav branch 3 times, most recently from 139247e to 317464e Compare December 5, 2024 19:20

gnprice reviewed Dec 5, 2024

View reviewed changes

PIG208 mentioned this pull request Dec 10, 2024

Series of prep commits for topic action sheet related changes #1125

Merged

gnprice reviewed Dec 11, 2024

View reviewed changes

PIG208 force-pushed the pr-nav branch from 5a2df22 to 2acf526 Compare December 11, 2024 03:12

PIG208 requested a review from gnprice December 11, 2024 03:12

gnprice reviewed Dec 11, 2024

View reviewed changes

PIG208 force-pushed the pr-nav branch from 2acf526 to 20cd331 Compare December 11, 2024 03:46

PIG208 requested a review from gnprice December 11, 2024 04:22

gnprice mentioned this pull request Dec 11, 2024

"About Zulip" in main menu #1128

Closed

PIG208 added 11 commits December 11, 2024 18:05

app: Apply titleSpacing for ChooseAccountPage's app bar

13c2044

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

login test [nfc]: Extract login helper

af76dc8

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

app test [nfc]: Convert initialRoutes into a generic helper

f83c795

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

login test [nfc]: Move takeStartingRoutes out of prepare

811ab86

This allows us to use `prepare` for testing the login page when the user is logged in. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

test store [nfc]: Support adding delay to loadPerAccount

63f8fe0

Signed-off-by: Zixuan James Li <zixuan@zulip.com>

nav [nfc]: Support custom loading page with AccountPageRouteMixin

215b51a

This preps for adding a special loading page for the home page. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

theme: Apply ElevatedButtonTheme globally

0968948

This applies to all the `ElevatedButton`'s, which are exclusively used in `HomePage`, `LoginPage`, `ChooseAccountPage`, and `AddAccountPage, to fix their color in dark mode. Signed-off-by: Zixuan James Li <zixuan@zulip.com>

app: Fix ChooseAccountPage's colors in dark mode

bd3b5b0

The colors are intended to match what we use for `ElevatedButton` (see `zulipThemeData`). Signed-off-by: Zixuan James Li <zixuan@zulip.com>

nav: Remove *Page widgets for page bodies that can be reached in Home…

46b30f7

…Page Fixes: zulip#1035 Signed-off-by: Zixuan James Li <zixuan@zulip.com>

PIG208 force-pushed the pr-nav branch from 20cd331 to 46b30f7 Compare December 11, 2024 23:09

gnprice merged commit 46b30f7 into zulip:main Dec 11, 2024
1 check passed

PIG208 deleted the pr-nav branch December 11, 2024 23:27

	// TODO: fetch organization icon and display name from the server settings
	// TODO(#1037): fetch organization icon and display name from the server settings

	~WidgetState.pressed: (selected) ? borderSide : null,
	~WidgetState.pressed: selected ? borderSide : null,

		return Scaffold(
		appBar: ZulipAppBar(title: const Text("Home")),

		/// The Zulip custom icon "contacts".
		static const IconData contacts = IconData(0xf109, fontFamily: "Zulip Icons");

	// TODO(design): determine a suitable max width
	// TODO(design): determine a suitable max width for bottom nav bar

	class NavigationButton extends StatelessWidget {
	class _NavigationBarButton extends StatelessWidget {

	child: Column(
	mainAxisAlignment: MainAxisAlignment.start,
	child: Column(

		Expanded(child: Text(label(zulipLocalizations),
		textAlign: TextAlign.start,

	check(find.byType(InboxPageBody)).findsOne();
	check(find.byType(InboxPageBody)).findsOne();
	check(find.byType(SubscriptionListPageBody)).findsNothing();


		Future<List<Route<dynamic>>> initialRoutes(WidgetTester tester) async {

	final loggedInAccount = testBinding.globalStore.accounts.singleWhere(
	final newAccount = testBinding.globalStore.accounts.singleWhere(

		final hasBackButton = ModalRoute.of(context)?.impliesAppBarDismissal ?? false;
		return MenuButtonTheme(

	late final ValueNotifier<HomePageTab> _tab = ValueNotifier(HomePageTab.inbox);
	late final _tab = ValueNotifier(HomePageTab.inbox);

	testWidgets('while loading, choosing an account disallow going back', (tester) async {
	testWidgets('while loading, choosing an account disallows going back', (tester) async {

		// TODO(#737): switch to a realistic setup:
		final account1Route = MaterialAccountWidgetRoute(

		return Icon(icon, size: _iconSize, color: selected ? designVariables.iconSelected
		: designVariables.icon);

Implement bottom tabs and main menu #1076

Implement bottom tabs and main menu #1076

Conversation

PIG208 commented Nov 23, 2024 • edited Loading

gnprice commented Nov 25, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Dec 4, 2024 • edited Loading

chrisbobbe commented Dec 4, 2024

PIG208 commented Dec 4, 2024

PIG208 commented Dec 5, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Nov 23, 2024 •

edited

Loading

gnprice commented Nov 25, 2024 •

edited

Loading

PIG208 Nov 26, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

PIG208 Dec 3, 2024 •

edited

Loading

PIG208 Dec 3, 2024 •

edited

Loading

PIG208 Dec 5, 2024 •

edited

Loading

gnprice Dec 3, 2024 •

edited

Loading

PIG208 Dec 4, 2024 •

edited

Loading

PIG208 commented Dec 4, 2024 •

edited

Loading

PIG208 commented Dec 5, 2024 •

edited

Loading