new_dm: Add UI for starting new DM conversations#1322
new_dm: Add UI for starting new DM conversations#1322chimnayajith wants to merge 1 commit intozulip:mainfrom
Conversation
b407738 to
d8818a2
Compare
|
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 |
0bc3e12 to
a6cc695
Compare
|
@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
left a comment
There was a problem hiding this comment.
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.
| width: 137, | ||
| height: 48, |
There was a problem hiding this comment.
| icon: Icon(Icons.add, size: 24), | ||
| label: Text(zulipLocalizations.newDmFabButtonLabel, style: TextStyle(fontSize: 20, fontWeight: FontWeight.w500)), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The extendedIconLabelSpacing field for extended floating action buttons has a default value of 8.0. Should it still be specified?
There was a problem hiding this comment.
Yeah. Because the default value can change (though unlikely), but we have a specific value in mind.
There was a problem hiding this comment.
Have given it in the revision pushed.
|
|
||
| void showNewDmSheet(BuildContext context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| showModalBottomSheet<dynamic>( |
There was a problem hiding this comment.
Let's avoid dynamic. We in general follow Flutter's style guide.
| @override | ||
| Widget build(BuildContext context) { | ||
| return SizedBox( | ||
| height: MediaQuery.of(context).size.height * 0.95, |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| class NewDmHeader extends StatelessWidget { |
There was a problem hiding this comment.
For helper widgets like this, let's make them private (_NewDmHeader) unless it is otherwise necessary.
| hintStyle: TextStyle( | ||
| fontSize: 17, | ||
| height: 1.0, | ||
| color: designVariables.textInput.withFadedAlpha(0.5)), |
There was a problem hiding this comment.
Looks like we do have a Figma variable for this: label-search-prompt.
| children: [ | ||
| Avatar(userId: userId, size: 22, borderRadius: 3), | ||
| Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 5, vertical: 3), |
There was a problem hiding this comment.
|
|
||
| final List<User> filteredUsers; | ||
| final Set<int> selectedUserIds; | ||
| final void Function(int) onUserSelected; |
There was a problem hiding this comment.
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.
| Widget build(BuildContext context) { | ||
| final designVariables = DesignVariables.of(context); | ||
|
|
||
| return ListView.builder( |
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 2), | ||
| child: Container( | ||
| padding: const EdgeInsets.symmetric(horizontal: 8, vertical: 6), |
| }); | ||
| } | ||
|
|
||
| // Scroll to the search field when the user selects a user |
There was a problem hiding this comment.
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.
| size: 24, | ||
| color: selectedUserIds.isEmpty | ||
| ? designVariables.icon.withFadedAlpha(0.5) | ||
| : designVariables.icon)])); |
There was a problem hiding this comment.
Square brackets are usually not wrapped, because they help indicate the end of a list and that the items before them are parallel.
| super.key, | ||
| required this.searchController, | ||
| required this.scrollController, | ||
| required this.selectedUserIds}); |
There was a problem hiding this comment.
Similarly, curly brackets are not wrapped, because they indicate the end of a parameter list, function, etc.
9439a4a to
e033db5
Compare
|
@PIG208 Pushed a revision. PTAL! |
|
@chimnayajith Are the screenshots in the PR description up to date? Please update them if not. |
|
I've updated the screenshots. Please take a look! |
|
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. |
|
Actually, it would probably be better to match the web app more fully, if it doesn't feel too long in the mobile context.
|
|
Why is there no "Add Person" in your last screenshot? What is different vs. the other ones? |
|
Continuing the discussion in CZO |
e033db5 to
126c4cc
Compare
126c4cc to
a8bcda8
Compare
|
@PIG208 Pushed a revision. PTAL! |
PIG208
left a comment
There was a problem hiding this comment.
Thanks for the revisions! Just a handful of comments.
|
|
||
| await tester.tap(userTileFinder); | ||
| await tester.pump(); | ||
|
|
There was a problem hiding this comment.
We should check nextButton here as well, so that we know that it's precisely the second tap that toggles the user selection
There was a problem hiding this comment.
Wouldn't the suggested change make the 'deselecting a user' test redundant with the 'selecting a user' test, since the latter already checks that nextButton.onTap becomes non-null after selection?
There was a problem hiding this comment.
Yeah, that's true. I think this probably suggests that these two tests can be combined together.
| await tester.pump(); | ||
| check(find.descendant( | ||
| of: selfUserTileFinder, | ||
| matching: find.byIcon(Icons.check_circle))).findsOne(); |
There was a problem hiding this comment.
We should check eg.selfUser.fullName here too.
| }); | ||
| testWidgets('navigates to group DM on Next', (tester) async { |
| tester, | ||
| users: [], | ||
| expectedAppBarTitle: 'DMs with yourself', | ||
| isSelfDm: true); |
There was a problem hiding this comment.
Hmm, how about just passing users: [eg.selfUser]? That way we don't need to introduce a separate parameter.
| final fabBgColor = _pressed | ||
| ? designVariables.fabBgPressed | ||
| : designVariables.fabBg; | ||
| final fabLabelColor = _pressed | ||
| ? designVariables.fabLabelPressed | ||
| : designVariables.fabLabel; | ||
| final fabShadowColor = _pressed | ||
| ? designVariables.fabShadowLow | ||
| : designVariables.fabShadowHigh; | ||
|
|
||
| return InkWell( | ||
| onTap: () => showNewDmSheet(context), | ||
| onTapDown: (_) => setState(() => _pressed = true), | ||
| onTapUp: (_) => setState(() => _pressed = false), | ||
| onTapCancel: () => setState(() => _pressed = false), | ||
| borderRadius: BorderRadius.circular(28), | ||
| child: AnimatedContainer( | ||
| duration: const Duration(milliseconds: 100), | ||
| curve: Curves.easeOut, | ||
| padding: const EdgeInsetsDirectional.fromSTEB(16, 12, 20, 12), | ||
| decoration: BoxDecoration( | ||
| color: fabBgColor, | ||
| borderRadius: BorderRadius.circular(28), | ||
| boxShadow: [ | ||
| BoxShadow( | ||
| // TODO(design): Shadow colors barely visible in dark mode | ||
| color: fabShadowColor, | ||
| blurRadius: _pressed ? 12 : 16, | ||
| spreadRadius: 0, | ||
| offset: _pressed | ||
| ? const Offset(0, 2) | ||
| : const Offset(0, 4)), | ||
| ]), |
There was a problem hiding this comment.
Let's make boxShadow (normal and pressed states) design variables. We can use these values in light mode, and simply null in dark mode. See ComposeBoxTheme for example.
There was a problem hiding this comment.
Looking at the design discussion on CZO thread it appears the decision is to have shadows in dark mode as well.
Given this, do I still need to follow the ComposeBoxTheme pattern with a separate theme extension class, or should I simply continue using designVariables.fabShadow directly in the button code as currently implemented?
There was a problem hiding this comment.
Good question. To be clear, my suggestion was to just extract them to DesignVariables, not a separate theme class.
We usually prefer matching the name of design variables in code to Figma. Now that we do have such a variable, and that they are not otherwise different in light vs. dark mode, it should be fine without refactoring.
|
@PIG208 Pushed a revision. PTAL! |
PIG208
left a comment
There was a problem hiding this comment.
Thanks! Just a comment on the l10n descriptions. Marking this for Greg's review.
| "description": "Hint text for the search bar when no users are selected" | ||
| }, | ||
| "newDmSheetSearchHintSomeSelected": "Add another user…", | ||
| "@newDmSheetSearchHintSomeSelected": { | ||
| "description": "Hint text for the search bar when at least one user is selected" |
There was a problem hiding this comment.
nit: missing period at the end of the sentence
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chimnayajith for building this, and thanks @PIG208 for the previous reviews!
Comments below. For this round, I've read the main new file down through the header and serach bar. Left for a future round are the user list, the changes in other widgets files, and the tests.
| if (user.fullName.toLowerCase().contains( | ||
| searchController.text.toLowerCase())) { |
There was a problem hiding this comment.
The searchController.text.toLowerCase() can be brought outside the loop, so that we do that computation just once instead of once per user.
| if (aLatestMessageId != null && bLatestMessageId != null) { | ||
| return bLatestMessageId.compareTo(aLatestMessageId); | ||
| } | ||
| if (aLatestMessageId != null) return -1; | ||
| if (bLatestMessageId != null) return 1; | ||
| return 0; |
There was a problem hiding this comment.
This is some fairly opaque logic. It's hard to tell if what it's doing is correct. A major reason why that's hard is that it's not explicit just what this is trying to do — so the reader has to guess, and then compare the logic they see to their guess at the intent, and if those don't match they can't be sure whether they've found a bug or their guess at the intent was wrong, so they may have to repeat several times with different guesses.
The main tactic for making this kind of code clear is to put it in its own method, with a clear name and doc comment.
We already have a method that does that, namely MentionAutocompleteView.compareRecentMessageIds, and its caller compareByDms which corresponds to this whole sort function. So it'd be best to just call MentionAutocompleteView.compareByDms.
| // TODO: switch to using an `AutocompleteView` for users | ||
| void _updateFilteredUsers(PerAccountStore store) { |
There was a problem hiding this comment.
Even though in this version we're not using the full machinery of AutocompleteView, let's do apply one of the key optimizations that MentionAutocompleteView makes:
- Have a
List<User> sortedUsers. Compute that just once, at the start of the interaction, with a full list of all users. - Then when the user edits their search filter, compute
filteredUsersby going through that list, without repeating the sorting.
|
|
||
| void _handleUserTap(int userId) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| final newSelectedUserIds = Set<int>.from(selectedUserIds); |
There was a problem hiding this comment.
Instead of copying this Set, we can mutate the existing one. We don't have a need for the old value.
| Navigator.pop(context); | ||
| Navigator.push(context, | ||
| MessageListPage.buildRoute(context: context, narrow: narrow)); |
There was a problem hiding this comment.
Perhaps Navigator.pushReplacement? How does the behavior differ between pop-then-push and that method?
| child: Row(children: [ | ||
| Expanded(child: Wrap( |
There was a problem hiding this comment.
Is this a Row that always has one child? What if we just use the child directly?
There was a problem hiding this comment.
But that would mean Expanded directly inside SingleChildScrollView which results in an error.
There was a problem hiding this comment.
Yeah, the Expanded is really part of the Row protocol — what I meant was to use the child of the Expanded directly.
It looks like Chris cleared this up as part of #1554:
a8751e4
So I guess what this Row-plus-Expanded was accomplishing was to make the Wrap take up the full width. That can be done more directly by setting CrossAxisAlignment.stretch on the parent Column.
| return Container( | ||
| constraints: const BoxConstraints( | ||
| minHeight: 44, | ||
| maxHeight: 124), | ||
| padding: const EdgeInsets.symmetric(horizontal: 14, vertical: 11), |
There was a problem hiding this comment.
Do these constraints apply to the size including the padding, or excluding it?
My guess is they include the padding. But I'm not sure. I'd have to study the Container docs, and/or implementation, to be sure.
Instead let's handle the constraints and the padding in two separate widgets. That way it'll be very clear right here in our code which one is on the outside of which.
| color: designVariables.labelSearchPrompt, | ||
| fontSize: 17, | ||
| height: 22 / 17), | ||
| border: InputBorder.none)); |
There was a problem hiding this comment.
nit: group properties logically; border is closely related to contentPadding (much more so than either of them is to the hint), so make them adjacent
| child: Row( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| Avatar(userId: userId, size: 22, borderRadius: 3), |
There was a problem hiding this comment.
nit: can make compact and reduce indentation:
| child: Row( | |
| mainAxisSize: MainAxisSize.min, | |
| children: [ | |
| Avatar(userId: userId, size: 22, borderRadius: 3), | |
| child: Row(mainAxisSize: MainAxisSize.min, children: [ | |
| Avatar(userId: userId, size: 22, borderRadius: 3), |
| Avatar(userId: userId, size: 22, borderRadius: 3), | ||
| Padding( | ||
| padding: const EdgeInsetsDirectional.fromSTEB(5, 3, 4, 3), | ||
| child: Text(user?.fullName ?? zulipLocalizations.unknownUserName, |
There was a problem hiding this comment.
We have a centralized getter for this. (It might be new since you began work on this feature.) Search for references to unknownUserName.
|
I have this running now in the build on my device. One quick comment from playing with it: when I select a user from the list of results, the search text I've entered should get cleared. Effectively by choosing a user I'm completing the name I had started typing as text. |
| fontSize: 20, | ||
| height: 30 / 20, | ||
| ).merge(weightVariableTextStyle(context, wght: 600)), | ||
| overflow: TextOverflow.ellipsis, |
There was a problem hiding this comment.
Does this overflow parameter have any effect without maxLines? Reading docs, I believe it doesn't.
|
Vlad offered some updates and feedback for the design of this page: Specifically I think items 0, 1, 3, 4, 5, 6 are things to be updated on this PR. (Item 5 is the same as my comment #1322 (comment) above.) Items 2, 7, and 8 are out of scope. Note also later discussion in that thread, in particular the updates to item 1 at #mobile > Flutter DMs flow design review @ 💬. |
|
Thanks again @chimnayajith for all your work on this PR! The new app's launch is just a few weeks away now (#mobile-team > Flutter beta timeline @ 💬), and this is one of the M5a launch-blocker issues we want to be sure to have completed before then. Since it looks like you don't currently have time to make revisions to this PR, I've asked @chrisbobbe to pick it up from here so we can be sure to have it finished soon. |
|
Sorry for the delay — I’ve been caught up with a few things, but I'm available now and can resume work on this starting today. If it's okay, I’d like to continue with the PR and make the necessary revisions. Let me know if that works! |
|
OK, that works 🙂 — please go ahead. We are on a tight timeline for this one (unlike our usual habits), so if at some point in the next week or two you're busy again, just let us know and we'll pick it up. |
6d756de to
f144ba6
Compare
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
|
Pushed a revision. PTAL.
Had a few doubts about some of these items. Started a CZO thread for the same. Also the |
|
Thanks @chimnayajith for the revision! It looks like @chrisbobbe has sent a new version, based on this revision, as #1554. So I'll close this PR in order to continue the thread there. (See also #1554 (comment) for Chris's detailed feedback on this revision, and #1554 (review) for my comments, many of which apply to this version.) |





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 theMessageListPage.Design reference: Figma
Related Issues
Screenshots
Light mode
Dark mode
Additional Notes
The user list is currently sorted based on the recency of direct messages.