zulip
new_dm: Add UI for starting new DM conversations
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 #127
Screenshots
Light mode
Dark mode
Additional Notes
The user list is currently sorted based on the recency of direct messages.
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.
@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.
@chimnayajith Are the screenshots in the PR description up to date? Please update them if not.
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.
- "Add one or more users" before anyone is selected
- "Add another user…" once someone is selected
Why is there no "Add Person" in your last screenshot? What is different vs. the other ones?
@PIG208 Pushed a revision with the mentioned changes in UI. PTAL
Note: Uses IntrinsicWidth for the search bar as discussed in CZ0 thread
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.
@Gaurav-Kushwaha-1225 Will be sending in a revision today, Got occupied with exams.
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.
@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
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.
Vlad offered some updates and feedback for the design of this page: #mobile > Flutter DMs flow design review @ 💬
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 https://github.com/zulip/zulip-flutter/pull/1322#issuecomment-2878199404 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 :slightly_smiling_face: — 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.
Pushed a revision. PTAL.
Specifically I think items 0, 1, 3, 4, 5, 6 are things to be updated on this PR.
Had a few doubts about some of these items. Started a CZO thread for the same.
Also the app_en.arb and other related files seems to already have an older version of the strings coming from this commit -> bb1ca88
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 https://github.com/zulip/zulip-flutter/pull/1554#issuecomment-2961170657 for Chris's detailed feedback on this revision, and https://github.com/zulip/zulip-flutter/pull/1554#pullrequestreview-2915241380 for my comments, many of which apply to this version.)