-
Notifications
You must be signed in to change notification settings - Fork 308
new-dm: Add UI for starting new DM conversation (Chris's revision) #1554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
To see my step-by-step changes atop @chimnayajith's work, check out my branch |
Filed #1555 for nicer text than "No earlier messages." for the common case where you use this flow to open a DM conversation that has no message history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chimnayajith and @chrisbobbe! This all looks good — just small comments below.
Going back through the comments Vlad made on v0.0.30 (#mobile > Flutter DMs flow design review @ 💬), I believe the current state is:
- 0 is in the followup PR home: Add placeholder text for empty Inbox, Channels, and Direct messages #1552 plus the followup issue msglist: Friendlier placeholder text when narrow has no messages #1555
- 1 is done here
- 2 is out of scope (not part of this new flow)
- 3 is done here
- 4 is done here
- 5 is done here
- 6 is tracked as a followup issue msglist: When the initial message fetch comes up empty, auto-focus the compose box #1543 and resolved in the followup PR msglist: When initial message fetch comes up empty, auto-focus compose box #1544
- 7 is out of scope (not part of this new flow)
- 8 is out of scope (surprisingly deep to fix: #mobile > Flutter DMs flow design review @ 💬)
/// The Zulip custom icon "check_circle_checked". | ||
static const IconData check_circle_checked = IconData(0xf109, fontFamily: "Zulip Icons"); | ||
|
||
/// The Zulip custom icon "check_circle_unchecked". | ||
static const IconData check_circle_unchecked = IconData(0xf10a, fontFamily: "Zulip Icons"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this diff happens in the commit after the one that adds the icons
lib/widgets/new_dm_sheet.dart
Outdated
isDense: true, | ||
border: InputBorder.none, | ||
contentPadding: EdgeInsets.zero, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: isDense
is closely related to contentPadding
, so keep together
isDense: true, | |
border: InputBorder.none, | |
contentPadding: EdgeInsets.zero, | |
isDense: true, | |
contentPadding: EdgeInsets.zero, | |
border: InputBorder.none, |
lib/widgets/new_dm_sheet.dart
Outdated
sortedUsers = List<User>.from(store.allUsers); | ||
sortedUsers.sort((a, b) => MentionAutocompleteView.compareByDms(a, b, store: store)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
sortedUsers = List<User>.from(store.allUsers); | |
sortedUsers.sort((a, b) => MentionAutocompleteView.compareByDms(a, b, store: store)); | |
sortedUsers = List<User>.from(store.allUsers) | |
..sort((a, b) => MentionAutocompleteView.compareByDms(a, b, store: store)); |
That way it gets set directly to the value we want it to have.
lib/widgets/new_dm_sheet.dart
Outdated
final user = filteredUsers[index]; | ||
final isSelected = selectedUserIds.contains(user.userId); | ||
|
||
return Material( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's pull this expression out as its own widget, say _NewDmUserListItem
; it'll cut a few levels of indentation, and I think it's a natural abstraction boundary
return Stack( | ||
alignment: Alignment.bottomCenter, | ||
clipBehavior: Clip.none, | ||
children:[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
children:[ | |
children: [ |
boxShadow: [BoxShadow( | ||
color: designVariables.fabShadow, | ||
blurRadius: _pressed ? 12 : 16, | ||
spreadRadius: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: leave out argument that equals the boring default
boxShadow: [BoxShadow( | |
color: designVariables.fabShadow, | |
blurRadius: _pressed ? 12 : 16, | |
spreadRadius: 0, | |
boxShadow: [BoxShadow( | |
color: designVariables.fabShadow, | |
blurRadius: _pressed ? 12 : 16, |
test/widgets/new_dm_sheet_test.dart
Outdated
} | ||
|
||
|
||
testWidgets('tapping user chip deselects the user', (tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
} | |
testWidgets('tapping user chip deselects the user', (tester) async { | |
} | |
testWidgets('tapping user chip deselects the user', (tester) async { |
test/widgets/new_dm_sheet_test.dart
Outdated
check(find.byType(ComposeBox)).findsOne(); | ||
} | ||
|
||
testWidgets('navigates to self DM on Next', (tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test name is out of date:
testWidgets('navigates to self DM on Next', (tester) async { | |
testWidgets('navigates to self DM', (tester) async { |
This was helpful, thanks. @chimnayajith I'd also highly recommend you check out that branch and read it commit by commit; it doubles as a detailed form of feedback on #1322 as of your latest revision. |
… a FAB We're about to add a "New DM" FAB, and this will ensure that the user can scoot the last few recent-DM items out from under that FAB.
(The _checked/_unchecked suffix is added; they're both called "checked_circle" in Figma.) When I use the SVGs from Figma without modifications, these look wrong; seems like the filled/unfilled areas are inverted, and the background is a solid square instead of transparent. Figma link: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=62-8121&m=dev So I deleted some goo that looks like it's not needed anyway. Here are the diffs: ``` @@ -1,10 +1,3 @@ <svg width="25" height="24" viewBox="0 0 25 24" fill="none" xmlns="http://www.w3.org/2000/svg"> -<g clip-path="url(#clip0_62_8120)"> <path d="M12.5 2C6.98 2 2.5 6.48 2.5 12C2.5 17.52 6.98 22 12.5 22C18.02 22 22.5 17.52 22.5 12C22.5 6.48 18.02 2 12.5 2ZM12.5 20C8.09 20 4.5 16.41 4.5 12C4.5 7.59 8.09 4 12.5 4C16.91 4 20.5 7.59 20.5 12C20.5 16.41 16.91 20 12.5 20Z" fill="black"/> -</g> -<defs> -<clipPath id="clip0_62_8120"> -<rect width="24" height="24" fill="white" transform="translate(0.5)"/> -</clipPath> -</defs> </svg> ``` ``` @@ -1,10 +1,3 @@ <svg width="25" height="24" viewBox="0 0 25 24" fill="none" xmlns="http://www.w3.org/2000/svg"> -<g clip-path="url(#clip0_62_8121)"> <path d="M12.5 2C6.98 2 2.5 6.48 2.5 12C2.5 17.52 6.98 22 12.5 22C18.02 22 22.5 17.52 22.5 12C22.5 6.48 18.02 2 12.5 2ZM9.79 16.29L6.2 12.7C5.81 12.31 5.81 11.68 6.2 11.29C6.59 10.9 7.22 10.9 7.61 11.29L10.5 14.17L17.38 7.29C17.77 6.9 18.4 6.9 18.79 7.29C19.18 7.68 19.18 8.31 18.79 8.7L11.2 16.29C10.82 16.68 10.18 16.68 9.79 16.29Z" fill="black"/> -</g> -<defs> -<clipPath id="clip0_62_8121"> -<rect width="24" height="24" fill="white" transform="translate(0.5)"/> -</clipPath> -</defs> </svg> ```
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 Co-authored-by: Chris Bobbe <[email protected]>
This isn't specifically mentioned in the Figma, but I think it's really helpful. Without this, the only way to deselect a user is to find them again in the list of results. That could be annoying because the user will often go offscreen in the results list as soon as you tap it, because we reset the query and scroll state when you tap it. Discussion about adding an "x" to the chip, to make this behavior more visible: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/Follow.20up.20on.20comments.20.20on.20new.20Dm.20Sheet/near/2185229
23e3be4
to
0058cd7
Compare
Thanks for the review! Revision pushed. |
Thanks! Looks good; merging. |
Many thanks to @chimnayajith for doing most of this work, in #1322!
As Greg foreshadowed in #1322 (comment), we're on a tight timeline for getting this feature in. I've gone ahead and made this revision directly, incorporating my own feedback, rather than going through another round of review.
I've sent other PRs for some things that could be separated out:
Fixes: #127