new_dm: Add UI for starting new DM conversations #1322

chimnayajith · 2025-02-04T15:14:43Z

Pull Request

Description

This pull request adds the UI to starting new DM conversations.. A floating action button has been added to the RecentDmConversationsPage, which opens a bottom modal sheet, allowing users to select conversation participants and proceed to the MessageListPage.

Design reference: Figma

Related Issues

Closes Support starting a new DM conversation #127

Screenshots

Light mode

Dark mode

Additional Notes

The user list is currently sorted based on the recency of direct messages.

PIG208 · 2025-02-12T23:04:56Z

Thanks for working on this @chimnayajith!

I took a quick look at the implementation and checked the design. There are places where it currently does not match the Figma, for example:

          Container(
            width: MediaQuery.of(context).size.width,
            constraints: const BoxConstraints(
              minHeight: 44,
              maxHeight: 124,
            ),
            padding: const EdgeInsets.symmetric(horizontal: 16, vertical: 8),
            decoration: BoxDecoration(
              color: designVariables.bgSearchInput,
            ),
            child: SingleChildScrollView(
              controller: scrollController,
              child: Row(
                children: [
                  Expanded(
                    child: Wrap(
                      spacing: 4,
                      runSpacing: 4,

where the horizontal padding should be 14px;

and the spacing between the pills should be 6px.

I think there might be more places like this, so please sure to check your PR with the design and make sure that they match exactly.

While working on the next revision, please also try to break down the sheet into reasonable widgets, instead of a single one that encompasses the entire new DM page. There are also things like collapsing the closing brackets into a single line; you can find examples of that throughout the codebase:

      // [...]
        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),
                  ])))))));

All those changes will help make your PR more reviewable.

At the high-level, I think the user filtering code should re-use the AutocompleteView API, similar to what EmojiPicker does. However, We don't have a implementation for user auto-completion. While MentionAutocompleteView is close, it is also responsible for other tasks. Supporting that might require a good amount of refactoring and is less suited for a new contributor. So let's just try to focus on getting the UI right for this PR.

chimnayajith · 2025-02-16T06:25:23Z

@PIG208 Thanks for the review! I’ve made the necessary changes to match the Figma, and refactored the sheet into smaller widgets. I also followed the code style guidelines you mentioned.

I’ve pushed the revision—PTAL.

PIG208

Thanks for the update! I went through the implementation of the design (mainly layout but not the colors) and left some comments. I haven't read the tests yet, but it should be a good amount of feedback to work on a new revision for.

PIG208 · 2025-02-21T19:49:23Z

lib/widgets/recent_dm_conversations.dart

+        width: 137,
+        height: 48,


Instead of setting the size definitely, let's express this in terms of paddings and minWidth/minHeight.

Because the "new dm" string can get translated (thus ends up having different width, or the user has a non-default font size, the actual width and height can vary.

PIG208 · 2025-02-21T19:51:00Z

lib/widgets/recent_dm_conversations.dart

+          icon: Icon(Icons.add, size: 24),
+          label: Text(zulipLocalizations.newDmFabButtonLabel, style: TextStyle(fontSize: 20, fontWeight: FontWeight.w500)),


Do we have a way to control/verify the spacing between the icon and the label? In Figma it's 8px but from this code it is not obvious if it complies to the design.

The extendedIconLabelSpacing field for extended floating action buttons has a default value of 8.0. Should it still be specified?

Yeah. Because the default value can change (though unlikely), but we have a specific value in mind.

Have given it in the revision pushed.

PIG208 · 2025-02-21T19:55:41Z

lib/widgets/new_dm_sheet.dart

+
+void showNewDmSheet(BuildContext context) {
+  final store = PerAccountStoreWidget.of(context);
+  showModalBottomSheet<dynamic>(


Let's avoid dynamic. We in general follow Flutter's style guide.

PIG208 · 2025-02-21T20:00:22Z

lib/widgets/new_dm_sheet.dart

+  @override
+  Widget build(BuildContext context) {
+    return SizedBox(
+      height: MediaQuery.of(context).size.height * 0.95,


It doesn't seem right to size it this way. If the goal is to avoid the top of the bottom sheet overlapping with the device's top inset, see useSafeArea on showModalBottomSheet.

PIG208 · 2025-02-21T20:02:59Z

lib/widgets/new_dm_sheet.dart

+  }
+}
+
+class NewDmHeader extends StatelessWidget {


For helper widgets like this, let's make them private (_NewDmHeader) unless it is otherwise necessary.

PIG208 · 2025-02-21T20:38:58Z

lib/widgets/new_dm_sheet.dart

+              hintStyle: TextStyle(
+                fontSize: 17,
+                height: 1.0,
+                color: designVariables.textInput.withFadedAlpha(0.5)),


Looks like we do have a Figma variable for this: label-search-prompt.

PIG208 · 2025-02-21T20:42:47Z

lib/widgets/new_dm_sheet.dart

+        children: [
+          Avatar(userId: userId, size: 22, borderRadius: 3),
+          Padding(
+            padding: const EdgeInsets.symmetric(horizontal: 5, vertical: 3),


The end inset is not 5px:

Suggested change

padding: const EdgeInsets.symmetric(horizontal: 5, vertical: 3),

padding: EdgeInsetsDirectional.fromSTEB(5, 3, 4, 3),

We use EdgeInsetsDirectional because the start/end insets are different. This is for RTL where left and right are flipped.

PIG208 · 2025-02-21T20:45:40Z

lib/widgets/new_dm_sheet.dart

+
+  final List<User> filteredUsers;
+  final Set<int> selectedUserIds;
+  final void Function(int) onUserSelected;


The name of this is a bit misleading, because "selected" seems to indicate that the user is added to the list of selected user, when in reality it is more like toggling the selection of the user.

How about something like void Function(int userId) onUserTapped, with the parameter name.

PIG208 · 2025-02-21T20:49:45Z

lib/widgets/new_dm_sheet.dart

+  Widget build(BuildContext context) {
+    final designVariables = DesignVariables.of(context);
+
+    return ListView.builder(


There is an 8px top padding that applies to the scrollable:

I assume that's something that allows you to scroll over. Perhaps ListView.padding will be helpful.

PIG208 · 2025-02-21T20:50:16Z

lib/widgets/new_dm_sheet.dart

+          child: Padding(
+            padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 2),
+            child: Container(
+              padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 6),


Is the outer Padding redundant?

PIG208 · 2025-02-21T20:58:00Z

lib/widgets/new_dm_sheet.dart

+    });
+  }
+
+  // Scroll to the search field when the user selects a user


Hmm, is this part of the UX specified in the design? It would be good to link to the relevant discussion/Figma if that's the case. Either way, I feel that it might be the best to leave this out from the initial implementation, to simplify things as we work it out.

PIG208 · 2025-02-21T21:04:31Z

lib/widgets/new_dm_sheet.dart

+          size: 24,
+          color: selectedUserIds.isEmpty
+            ? designVariables.icon.withFadedAlpha(0.5)
+            : designVariables.icon)]));


Square brackets are usually not wrapped, because they help indicate the end of a list and that the items before them are parallel.

PIG208 · 2025-02-21T21:05:12Z

lib/widgets/new_dm_sheet.dart

+    super.key,
+    required this.searchController,
+    required this.scrollController,
+    required this.selectedUserIds});


Similarly, curly brackets are not wrapped, because they indicate the end of a parameter list, function, etc.

chimnayajith · 2025-02-25T15:17:09Z

@PIG208 Pushed a revision. PTAL!

alya · 2025-02-26T23:42:47Z

@chimnayajith Are the screenshots in the PR description up to date? Please update them if not.

chimnayajith · 2025-02-28T08:32:22Z

I've updated the screenshots. Please take a look!

alya · 2025-02-28T17:18:44Z

Let's change "Add Person" to "Add user", which will be more consistent with the web app. I'm not sure why your screenshots have varied capitalization, but in any case, only the first word should be capitalized.

alya · 2025-02-28T17:21:38Z

Actually, it would probably be better to match the web app more fully, if it doesn't feel too long in the mobile context.

"Add one or more users" before anyone is selected
"Add another user…" once someone is selected

alya · 2025-02-28T17:23:32Z

Why is there no "Add Person" in your last screenshot? What is different vs. the other ones?

chimnayajith · 2025-02-28T18:09:15Z

Continuing the discussion in CZO

chimnayajith · 2025-03-15T06:28:49Z

@PIG208 Pushed a revision with the mentioned changes in UI. PTAL

Note: Uses IntrinsicWidth for the search bar as discussed in CZ0 thread

gnprice

Thanks @chimnayajith!

This is still in maintainer review with @PIG208, but I took a quick skim just now and also tried playing with this in the running app. Here's one high-level comment on the code, and a couple of things that jumped out at me in the UX.

gnprice · 2025-03-18T00:39:30Z

lib/widgets/recent_dm_conversations.dart

-    return SafeArea(
+    return Scaffold(
+      body: SafeArea(


This will mean a Scaffold nested inside a Scaffold. See the Scaffold docs on that:
https://main-api.flutter.dev/flutter/material/Scaffold-class.html#nested-scaffolds

Have you tried alternatives to nesting the Scaffolds?

gnprice · 2025-03-18T00:48:48Z

lib/widgets/recent_dm_conversations.dart

+            zulipLocalizations.newDmFabButtonLabel,
+            style: const TextStyle(fontSize: 20, height: 24 / 20)
+              .merge(weightVariableTextStyle(context, wght: 500))),
+          backgroundColor: designVariables.fabBg,


Trying out the PR, this color doesn't look right.

It looks like the screenshots in the PR description agree with what I see running it (good), but don't agree on this color with what's in Figma.

gnprice · 2025-03-18T00:50:10Z

lib/widgets/recent_dm_conversations.dart

+      floatingActionButton: Container(
+        constraints: const BoxConstraints(
+          minWidth: 137,
+          minHeight: 48),


Where do these numbers come from?

When I run the PR, the button looks too big, with oversized padding:

Yeah, most of the time, as long as we get the paddings right, the size of the button should match the Figma design with normal UI scaling setting, so the constraints is not needed.

PIG208

Thanks for the update! I went through the implementation but haven't looked at the tests in this round. Most of the colors look good to me. The font height/size/weight also looks good. Once all the comments are addressed, this should match the design quite well. Using devtool will be helpful to confirm the actual sizes/paddings.

PIG208 · 2025-03-18T16:16:54Z

lib/widgets/new_dm_sheet.dart

+        child: NewDmPicker(pageContext: context))));
+}
+
+class NewDmPicker extends StatefulWidget {


nit: for NewDmPicker, we can add a @visibleForTesting annotation since we only use it for testing externally.

PIG208 · 2025-03-18T16:23:45Z

lib/widgets/new_dm_sheet.dart

+                  padding: const EdgeInsets.all(8),
+                  child: Icon(
+                    isSelected ? Icons.check_circle : Icons.circle_outlined,
+                    color: isSelected ? designVariables.radioFillSelected : designVariables.radioBorder,


This line is too wide (>80 characters); consider breaking this into multiple lines.

PIG208 · 2025-03-18T16:23:58Z

lib/widgets/new_dm_sheet.dart

+              ])));
+      });
+  }
+}


nit: add a newline at the end of the file

PIG208 · 2025-03-18T16:44:10Z

lib/widgets/new_dm_sheet.dart

+class NewDmPicker extends StatefulWidget {
+  const NewDmPicker({
+    super.key,
+    required this.pageContext


nit: need a trailing comma here

PIG208 · 2025-03-18T16:44:30Z

lib/widgets/new_dm_sheet.dart

+  const _NewDmUserList({
+    required this.filteredUsers,
+    required this.selectedUserIds,
+    required this.onUserTapped


nit: similar issue with missing trailing comma

PIG208 · 2025-03-19T21:30:51Z

lib/widgets/new_dm_sheet.dart

+  Widget build(BuildContext context) {
+    final designVariables = DesignVariables.of(context);
+    final store = PerAccountStoreWidget.of(context);
+    final user = store.getUser(userId)!;


Hmm, I don't feel confident that the userId will always present. If it does, we need a comment justifying why it is always present; if it doesn't, we should be more defensive and find a way to not throw.

PIG208 · 2025-03-19T21:31:35Z

lib/widgets/new_dm_sheet.dart

+    final user = store.getUser(userId)!;
+
+    return Container(
+    decoration: BoxDecoration(color: designVariables.bgMenuButtonSelected, borderRadius: BorderRadius.circular(3)),


nit: missing indentation; this line should also be wrapped because it is too long (>80 characters)

PIG208 · 2025-03-19T21:39:40Z

lib/widgets/new_dm_sheet.dart

+      children: [
+        _buildBackButton(context),
+        Text(zulipLocalizations.newDmSheetScreenTitle,
+          style: TextStyle(color: designVariables.title,
+            fontSize: 20, height: 30 / 20)
+            .merge(weightVariableTextStyle(context, wght: 600))),
+        _buildNextButton(context),


This overflows on the right with a large font size setting:

The simplest workaround is probably just make the center element Expanded. It might be tricky to get it right, so let's leave the full fix for later.

PIG208 · 2025-03-19T21:41:57Z

lib/widgets/new_dm_sheet.dart

+        final isSelected = selectedUserIds.contains(user.userId);
+
+        return InkWell(
+          onTap: () => onUserTapped(user.userId),


Let's use splashFactory: NoSplash.splashFactory, here; in general we avoid having Material's splash effect in the app.

PIG208 · 2025-03-19T21:50:10Z

lib/widgets/new_dm_sheet.dart

+                Avatar(userId: user.userId, size: 32, borderRadius: 3),
+                Expanded(
+                  child: Padding(
+                    padding: const EdgeInsetsDirectional.fromSTEB(8, 6.5, 0, 6.5),
+                    child: Text(user.fullName,
+                      style: TextStyle(
+                        fontSize: 17,
+                        height: 19 / 17,
+                        color: designVariables.textMessage)
+                      .merge(weightVariableTextStyle(context, wght: 500))))),
+              ])));


This group of widgets should have some vertical and right paddings:

I see that the leading check_circle icon is the tallest widget in the row by default, but that might not be the case with non-default font setting.

Gaurav-Kushwaha-1225 · 2025-04-01T06:07:01Z

Hii @chimnayajith,
The PR seems to be silent for 2 weeks. Are you still working on it?

Since already a lot of reviews and updates have gone through the process. If you are not able to take this further or need some help, I will be happy to join this and proceed further.

chimnayajith · 2025-04-01T15:55:25Z

@Gaurav-Kushwaha-1225 Will be sending in a revision today, Got occupied with exams.

chimnayajith · 2025-04-01T19:37:19Z

@PIG208 Pushed a revision. PTAL.

PIG208

Thanks! This looks a lot closer to the design now and I left some small comments on the implementation. This time, I also reviewed the tests.

PIG208 · 2025-04-07T20:33:15Z

lib/widgets/new_dm_sheet.dart

+              child: Text(zulipLocalizations.newDmSheetScreenTitle,
+                style: TextStyle(color: designVariables.title,
+                  fontSize: 20, height: 30 / 20)
+                  .merge(weightVariableTextStyle(context, wght: 600)),


nit: indentation

PIG208 · 2025-04-07T20:33:37Z

lib/widgets/new_dm_sheet.dart

+}
+
+class _SelectedUserChip extends StatelessWidget {
+  const _SelectedUserChip({ required this.userId });


nit: remove spaces around the parameter

PIG208 · 2025-04-07T20:35:56Z

lib/widgets/theme.dart

@@ -197,6 +203,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
    contextMenuItemMeta: const Color(0xff9194a3),
    contextMenuItemText: const Color(0xff9398fd),
    editorButtonPressedBg: Colors.white.withValues(alpha: 0.06),
+    fabBg: const Color(0xff4f42c9),
+    fabBgPressed: const Color(0xff6159e1),


The dark mode color of this doesn't match the design:

PIG208 · 2025-04-07T20:37:38Z

lib/widgets/theme.dart

@@ -197,6 +203,9 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
    contextMenuItemMeta: const Color(0xff9194a3),
    contextMenuItemText: const Color(0xff9398fd),
    editorButtonPressedBg: Colors.white.withValues(alpha: 0.06),
+    fabBg: const Color(0xff4f42c9),
+    fabBgPressed: const Color(0xff6159e1),
+    fabLabel: const Color(0xffeceefc),


There is also a pressed color variant for fabLabel:

PIG208 · 2025-04-07T20:39:05Z

test/widgets/new_dm_sheet_test.dart

+      await runAndCheck(tester, user: user);
+    });
+  });
+}


nit: need a newline after this

PIG208 · 2025-04-07T21:26:02Z

test/widgets/new_dm_sheet_test.dart

+    }
+
+    testWidgets('navigates to message list on Next', (tester) async {
+      final user = eg.user(userId: 1, fullName: 'Test User');


We can skip specifying userId because we don't rely on it being this particular value for the test.

PIG208 · 2025-04-07T21:26:29Z

test/widgets/new_dm_sheet_test.dart

+        eg.user(userId: 1, fullName: 'Alice Anderson'),
+        eg.user(userId: 2, fullName: 'Bob Brown'),
+        eg.user(userId: 3, fullName: 'Alice Carter'),


nit: we can skip specifying user IDs here

PIG208 · 2025-04-07T21:27:12Z

test/widgets/new_dm_sheet_test.dart

+  for (final user in users) {
+    await store.addUser(user);
+  }


nit: use store.addUsers instead

PIG208 · 2025-04-07T21:33:32Z

test/widgets/new_dm_sheet_test.dart

+      check(nextButton.onTap).isNull();
+    });
+
+    testWidgets('shows filtered users based on search', (tester) async {


I think there are some other interesting filtering logic that can be tested. Such as checking that the self user is excluded from the list, the search is case-insensitive. Another useful test would be checking if unknown users are handled correctly.

I wasn't able to write tests specifically for handling unknown users, but the logic relies on userDisplayName, which is already covered in user_test.dart. I also didn’t find any other tests that handle this specifically.

I realized that it's a product question as to whether we want to allow selecting the self user. I think we should, since that's allowed on web and the legacy app.

PIG208 · 2025-04-07T21:40:36Z

lib/widgets/new_dm_sheet.dart

+  });
+
+  final BuildContext pageContext;
+  final PerAccountStore store;


I think we can skip passing this by replacing the _updateFilteredUsers call with an overridden didChangeDependencies.

@override void didChangeDependencies() { super.didChangeDependencies(); _handleSearchUpdate(); }

PIG208 · 2025-04-17T19:32:02Z

Hi @chimnayajith! Thanks for working on this. Since this issue is a launch blocker, we would prioritize reviewing this/getting this done first; the other issue that you are working on (#1344) is a launch goal, which means that it will likely receive less attention until we clear the blockers.

chimnayajith · 2025-04-20T20:14:48Z

@PIG208 Pushed a revision. PTAL.

I had missed the "No users found" screen earlier; just added it and included the screenshots in the PR description. There aren’t any design references for it, but it follows how it’s done in the RN app

PIG208 · 2025-04-21T21:11:15Z

lib/widgets/new_dm_sheet.dart

+      return Center(
+        child: Padding(
+          padding: const EdgeInsets.all(24),
+          child: Text(
+            zulipLocalizations.newDmSheetNoUsersFound,
+            style: TextStyle(
+              color: designVariables.labelMenuButton,
+              fontSize: 16))));


This will need a TODO(design) comment since it is not a part of the Figma design.

PIG208 · 2025-04-21T21:12:06Z

lib/widgets/new_dm_sheet.dart

+            decoration: isSelected ? BoxDecoration(
+              color: designVariables.bgMenuButtonSelected,
+              borderRadius: BorderRadius.circular(10)) : null,


nit: Container shouldn't be necessary; we can use DecoratedBox

another nit: I think we usually have the null case first, for reasons similar to why early returns are preferred.

PIG208 · 2025-04-21T21:13:47Z

lib/widgets/new_dm_sheet.dart

+                          color: designVariables.textMessage)
+                            .merge(weightVariableTextStyle(context, wght: 500))))),


nit:

Suggested change

color: designVariables.textMessage)

.merge(weightVariableTextStyle(context, wght: 500))))),

color: designVariables.textMessage,

).merge(weightVariableTextStyle(context, wght: 500))))),

This way the end boundary of TextStyle is clear.

PIG208 · 2025-04-21T21:19:28Z

test/widgets/new_dm_sheet_test.dart

+      check(nextButton.onTap).isNull();
+    });
+
+    testWidgets('shows filtered users based on search', (tester) async {


I realized that it's a product question as to whether we want to allow selecting the self user. I think we should, since that's allowed on web and the legacy app.

PIG208 · 2025-04-21T21:20:08Z

test/widgets/new_dm_sheet_test.dart

+        if (users.length == 1) {
+          check(find.descendant(
+            of: find.byType(ZulipAppBar),
+            matching: find.text('DMs with ${users[0].fullName}'))).findsOne();
+        } else {
+          final names = users.map((user) => user.fullName).join(', ');
+          check(find.descendant(
+            of: find.byType(ZulipAppBar),
+            matching: find.text('DMs with $names'))).findsOne();
+        }


It will probably be easier to pass the expected app bar title from the tests.

PIG208

Thanks for the update! We have gone through the design in previous rounds. In this review I focused mostly on new changes like _NewDmButton and the tests.

PIG208 · 2025-04-22T02:11:27Z

lib/widgets/recent_dm_conversations.dart

+      onTapUp: (_) => setState(() => _pressed = false),
+      onTapCancel: () => setState(() => _pressed = false),
+      borderRadius: BorderRadius.circular(28),
+      child: Container(


Let's use the animated version AnimatedContainer here. The Figma design has specified the duration and curve for transitions.

Looks like it also handles the box shadow animation properly, so we can just complete the TODO from #1322 (comment), now that we do have a AnimatedScaleOnTap style widget.

PIG208 · 2025-04-22T02:13:58Z

lib/widgets/recent_dm_conversations.dart

+              zulipLocalizations.newDmFabButtonLabel,
+              style: TextStyle(fontSize: 20, height: 24 / 20)
+                .merge(weightVariableTextStyle(context, wght: 500))
+                .copyWith(color: fabLabelColor)),


It should be fine to just specify the color when constructing TextStyle, right?

PIG208 · 2025-04-22T02:18:09Z

lib/widgets/recent_dm_conversations.dart

@@ -138,3 +149,55 @@ class RecentDmConversationsItem extends StatelessWidget {
          ]))));
  }
 }
+
+class _NewDmButton extends StatefulWidget {
+  final VoidCallback onPressed;


nit: inline the callback in the _NewDmButtonState.build so that we can remove this parameter; it seems fitting that this widget is responsible for calling showNewDmSheet

PIG208 · 2025-04-22T02:19:22Z

lib/widgets/recent_dm_conversations.dart

+    final fabBgColor = _pressed ? designVariables.fabBgPressed : designVariables.fabBg;
+    final fabLabelColor = _pressed ? designVariables.fabLabelPressed : designVariables.fabLabel;


nit: these lines are too long; wrap them so that they stay under 80 characters in width

PIG208 · 2025-04-22T02:20:55Z

test/widgets/new_dm_sheet_test.dart

+import 'test_app.dart';
+
+Future<void> setupSheet(WidgetTester tester, {
+  required List<User> users


nit: missing trailing comma

PIG208 · 2025-04-22T02:32:36Z

test/widgets/new_dm_sheet_test.dart

+      testWidgets('selecting a user', (tester) async {
+        final user = eg.user(fullName: 'Test User');
+        await setupSheet(tester, users: [user]);
+        Finder findNextButton() => find.widgetWithText(GestureDetector, 'Next');
+
+        check(find.byIcon(Icons.circle_outlined)).findsOne();
+        check(find.byIcon(Icons.check_circle)).findsNothing();
+
+        var nextButton = tester.widget<GestureDetector>(findNextButton());
+        check(nextButton.onTap).isNull();
+        final userTile = find.ancestor(
+          of: find.text(user.fullName),
+          matching: find.byType(InkWell));
+        await tester.tap(userTile);
+        await tester.pump();
+        check(find.byIcon(Icons.check_circle)).findsOne();
+        check(find.byIcon(Icons.circle_outlined)).findsNothing();
+        nextButton = tester.widget<GestureDetector>(findNextButton());
+        check(nextButton.onTap).isNotNull();
+      });


nit:

Suggested change

testWidgets('selecting a user', (tester) async {

final user = eg.user(fullName: 'Test User');

await setupSheet(tester, users: [user]);

Finder findNextButton() => find.widgetWithText(GestureDetector, 'Next');

check(find.byIcon(Icons.circle_outlined)).findsOne();

check(find.byIcon(Icons.check_circle)).findsNothing();

var nextButton = tester.widget<GestureDetector>(findNextButton());

check(nextButton.onTap).isNull();

final userTile = find.ancestor(

of: find.text(user.fullName),

matching: find.byType(InkWell));

await tester.tap(userTile);

await tester.pump();

check(find.byIcon(Icons.check_circle)).findsOne();

check(find.byIcon(Icons.circle_outlined)).findsNothing();

nextButton = tester.widget<GestureDetector>(findNextButton());

check(nextButton.onTap).isNotNull();

});

testWidgets('selecting a user', (tester) async {

final user = eg.user(fullName: 'Test User');

final userTileFinder = find.ancestor(

of: find.text(user.fullName),

matching: find.byType(InkWell));

await setupSheet(tester, users: [user]);

final nextButton = tester.widget<GestureDetector>(

find.widgetWithText(GestureDetector, 'Next'));

check(find.byIcon(Icons.circle_outlined)).findsOne();

check(find.byIcon(Icons.check_circle)).findsNothing();

check(nextButton.onTap).isNull();

await tester.tap(userTileFinder);

await tester.pump();

check(find.byIcon(Icons.circle_outlined)).findsNothing();

check(find.byIcon(Icons.check_circle)).findsOne();

check(nextButton.onTap).isNotNull();

});

By breaking the checks into two stanzas, we can make this a bit more readable. (We don't need to call tester.widget() twice to see updates to nextButton.onTap.)

We don't need to call tester.widget() twice to see updates to nextButton.onTap.

I tried implementing this change, but I'm running into some issues - the tests are failing when I remove the second call.
When I only get the button once before the tap and then check its onTap property after the UI update, the test fails as if the property isn't being updated.
Could you help me understand the correct approach here?

I see. Yeah, it looks like the return values of the tester.widget call before and after the pump are different. Let's keep calling them again then.

PIG208 · 2025-04-22T02:37:06Z

test/widgets/recent_dm_conversations_test.dart

+
+    testWidgets('opens new DM sheet on FAB tap', (tester) async {
+      await setupPage(tester, users: [], dmMessages: []);
+      final fab = find.ancestor(


nit: Let's avoid abbreviations like this (here and in the test name) when possible, unless it comes from an outside source like the Figma design. In this case, we are just referring to the new DM button widget.

Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4931-2269&t=ZceQDxxhPnduqqWe-4

chimnayajith · 2025-04-28T14:27:31Z

@PIG208 Pushed a revision. PTAL!

PIG208

Thanks for the update! Left some comments, most are small, except for ones on adding more test cases.

PIG208 · 2025-04-28T19:35:38Z

lib/widgets/new_dm_sheet.dart

+      bool shouldIncludeSelfUser(User user) {
+        final shouldExcludeSelfUser = selectedUserIds.isNotEmpty
+          && !selectedUserIds.contains(store.selfUserId);
+
+        if (user.userId != store.selfUserId) return true;
+        return !shouldExcludeSelfUser;
+      }


This seems a bit complicated. From reading this, it seems that if user is selfUserId, this will only return false when selectedUserIds is non-empty and contains selfUserId.

However, I think the shouldIncludeUser(user) check can just be removed from the .where condition, since the intention is to just not special-case self-user when filtering by user name.

PIG208 · 2025-04-28T19:39:41Z

lib/widgets/new_dm_sheet.dart

+        if (userId != store.selfUserId
+          && selectedUserIds.contains(store.selfUserId)) {
+          selectedUserIds.remove(store.selfUserId);
+        }


Similarly, it will probably be simpler if we don't special-case self user ID. The UX here seems a bit confusing when I tried it out: if myself is selected, then I go on to select another user, myself is deselected. This seemed a bit surprising.

PIG208 · 2025-04-28T19:41:33Z

lib/widgets/new_dm_sheet.dart

+            decoration: !isSelected
+              ? const BoxDecoration()
+              : BoxDecoration(color: designVariables.bgMenuButtonSelected,
+                borderRadius: BorderRadius.circular(10)),


nit: indentation

PIG208 · 2025-04-28T19:48:33Z

lib/widgets/new_dm_sheet.dart

+                borderRadius: BorderRadius.circular(10)),
+            child: Padding(
+              padding: const EdgeInsets.fromLTRB(0, 6, 12, 6),
+              child:Row(


nit: missing space

PIG208 · 2025-04-28T19:51:28Z

lib/widgets/new_dm_sheet.dart

+        final narrow = DmNarrow.withOtherUsers(
+          selectedUserIds,
+          selfUserId: store.selfUserId);


Because now selectedUserIds can contain selfUserId, this needs to change. See the dartdoc of DmNarrow.withOtherUsers.

PIG208 · 2025-04-28T19:56:49Z

test/widgets/new_dm_sheet_test.dart

+
+        check(find.byType(ComposeBox)).findsOne();
+      }
+


Let's also test the cases when only the selfUser is selected, and when the selfUser is selected with some other users.

PIG208 · 2025-04-28T20:05:11Z

test/widgets/new_dm_sheet_test.dart

+      testWidgets('selecting a user', (tester) async {
+        final user = eg.user(fullName: 'Test User');
+        await setupSheet(tester, users: [user]);
+        Finder findNextButton() => find.widgetWithText(GestureDetector, 'Next');
+
+        check(find.byIcon(Icons.circle_outlined)).findsOne();
+        check(find.byIcon(Icons.check_circle)).findsNothing();
+
+        var nextButton = tester.widget<GestureDetector>(findNextButton());
+        check(nextButton.onTap).isNull();
+        final userTile = find.ancestor(
+          of: find.text(user.fullName),
+          matching: find.byType(InkWell));
+        await tester.tap(userTile);
+        await tester.pump();
+        check(find.byIcon(Icons.check_circle)).findsOne();
+        check(find.byIcon(Icons.circle_outlined)).findsNothing();
+        nextButton = tester.widget<GestureDetector>(findNextButton());
+        check(nextButton.onTap).isNotNull();
+      });


I see. Yeah, it looks like the return values of the tester.widget call before and after the pump are different. Let's keep calling them again then.

PIG208 · 2025-04-28T20:08:45Z

lib/widgets/new_dm_sheet.dart

+      filteredUsers = store.allUsers
+        .where((user) =>
+          shouldIncludeSelfUser(user) &&
+          !user.isBot &&


I think we shouldn't exclude bot users. Some bots might actually utilize DMs as a way of receiving commands from the user.

PIG208 · 2025-04-28T20:10:25Z

lib/widgets/new_dm_sheet.dart

+              textAlign: TextAlign.center)),
+          _buildNextButton(context),
+        ])
+    );


nit: move trailing parenthesis to the previous line

PIG208 · 2025-04-28T20:18:56Z

lib/widgets/recent_dm_conversations.dart

+                ? const Color(0xff2B0E8A).withValues(alpha: 0.3)
+                : const Color(0xff2B0E8A).withValues(alpha: 0.4),


Hmm, so these are not named variables in the Figma design. In dark mode, the shadow is barely visible. Let's also have a TODO(design) comment here, mentioning dark mode colors. Perhaps it is also helpful to discussion in #mobile-design on whether we want the shadows in dark mode or not.

Opened a design thread for this in CZO here

chimnayajith · 2025-04-29T04:14:42Z

Started a CZO thread to discuss the filtering logic.

Add a modal bottom sheet UI for starting direct messages: - Search and select users from global list - Support single and group DMs - Navigate to message list after selection Design reference: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4903-31879&p=f&t=pQP4QcxpccllCF7g-0 Fixes: zulip#127

chimnayajith · 2025-05-03T04:15:11Z

@PIG208 Pushed a revision. PTAL!

gnprice added the maintainer review PR ready for review by Zulip maintainers label Feb 4, 2025

gnprice assigned PIG208 Feb 4, 2025

chimnayajith force-pushed the 127-new-dm branch 4 times, most recently from b407738 to d8818a2 Compare February 6, 2025 18:21

PIG208 self-requested a review February 12, 2025 20:46

chimnayajith force-pushed the 127-new-dm branch 3 times, most recently from 0bc3e12 to a6cc695 Compare February 16, 2025 06:23

PIG208 reviewed Feb 21, 2025

View reviewed changes

chimnayajith force-pushed the 127-new-dm branch 3 times, most recently from 9439a4a to e033db5 Compare February 24, 2025 16:10

chimnayajith requested a review from PIG208 February 25, 2025 15:16

chimnayajith force-pushed the 127-new-dm branch from e033db5 to 126c4cc Compare February 28, 2025 18:30

Dhanesh-Sawant mentioned this pull request Mar 12, 2025

new dm : page to start a new dm conversation #1252

Closed

chimnayajith force-pushed the 127-new-dm branch from 126c4cc to a8bcda8 Compare March 15, 2025 05:17

gnprice reviewed Mar 18, 2025

View reviewed changes

PIG208 reviewed Mar 19, 2025

View reviewed changes

chimnayajith force-pushed the 127-new-dm branch 2 times, most recently from 49a7fbe to 30b556d Compare April 1, 2025 19:37

PIG208 reviewed Apr 7, 2025

View reviewed changes

chimnayajith force-pushed the 127-new-dm branch 2 times, most recently from 61ede77 to 82b1b66 Compare April 20, 2025 20:13

PIG208 reviewed Apr 21, 2025

View reviewed changes

PIG208 reviewed Apr 22, 2025

View reviewed changes

chimnayajith force-pushed the 127-new-dm branch from 82b1b66 to 161e5c5 Compare April 28, 2025 09:43

icons: Add chevron_left icon

6b5f189

Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4931-2269&t=ZceQDxxhPnduqqWe-4

chimnayajith force-pushed the 127-new-dm branch 3 times, most recently from d221599 to 7eb7984 Compare April 28, 2025 14:06

PIG208 reviewed Apr 28, 2025

View reviewed changes

chimnayajith force-pushed the 127-new-dm branch from 7eb7984 to 76ccab3 Compare May 2, 2025 18:57

		icon: Icon(Icons.add, size: 24),
		label: Text(zulipLocalizations.newDmFabButtonLabel, style: TextStyle(fontSize: 20, fontWeight: FontWeight.w500)),

	padding: const EdgeInsets.symmetric(horizontal: 5, vertical: 3),
	padding: EdgeInsetsDirectional.fromSTEB(5, 3, 4, 3),

		color: designVariables.textMessage)
		.merge(weightVariableTextStyle(context, wght: 500))))),

		final fabBgColor = _pressed ? designVariables.fabBgPressed : designVariables.fabBg;
		final fabLabelColor = _pressed ? designVariables.fabLabelPressed : designVariables.fabLabel;

		? const Color(0xff2B0E8A).withValues(alpha: 0.3)
		: const Color(0xff2B0E8A).withValues(alpha: 0.4),

new_dm: Add UI for starting new DM conversations #1322

Are you sure you want to change the base?

new_dm: Add UI for starting new DM conversations #1322

Conversation

chimnayajith commented Feb 4, 2025 • edited Loading

Pull Request

Description

Related Issues

Screenshots

Light mode

Dark mode

Additional Notes

PIG208 commented Feb 12, 2025 • edited Loading

chimnayajith commented Feb 16, 2025

PIG208 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

PIG208 Feb 25, 2025 • 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

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

chimnayajith commented Feb 25, 2025

alya commented Feb 26, 2025

chimnayajith commented Feb 28, 2025 • edited Loading

alya commented Feb 28, 2025 • edited Loading

alya commented Feb 28, 2025 • edited Loading

alya commented Feb 28, 2025

chimnayajith commented Feb 28, 2025

chimnayajith commented Mar 15, 2025 • 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

PIG208 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

Gaurav-Kushwaha-1225 commented Apr 1, 2025 • edited Loading

chimnayajith commented Apr 1, 2025 • edited Loading

chimnayajith commented Apr 1, 2025

PIG208 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Apr 17, 2025

chimnayajith commented Apr 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chimnayajith commented Feb 4, 2025 •

edited

Loading

PIG208 commented Feb 12, 2025 •

edited

Loading

PIG208 Feb 25, 2025 •

edited

Loading

chimnayajith commented Feb 28, 2025 •

edited

Loading

alya commented Feb 28, 2025 •

edited

Loading

alya commented Feb 28, 2025 •

edited

Loading

chimnayajith commented Mar 15, 2025 •

edited

Loading

Gaurav-Kushwaha-1225 commented Apr 1, 2025 •

edited

Loading

chimnayajith commented Apr 1, 2025 •

edited

Loading

chimnayajith commented Apr 20, 2025 •

edited

Loading