-
-
Notifications
You must be signed in to change notification settings - Fork 313
refactor(navigation)!: Make contacts screen a list/detail view #3010
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
The contacts screen has been refactored to use a list/detail pattern. The `ContactsScreen` composable has been replaced with `Contacts` and now manages the display of both the list of conversations and the detail view for a selected conversation using `NavigableListDetailPaneScaffold`. The `ContactsRoutes.Contacts` route has been removed and `ContactsRoutes.Messages` is now the entry point for the contacts graph. The `MessageScreen` is now displayed in the detail pane. The `MainAppBar` composable has been renamed to `GlobalAppBar` and the previously private `MainAppBar` component is now public. Deep link URI for messages has been updated to use query parameters. Signed-off-by: James Rich <[email protected]>
I still really want to sort the naming around all of this - messages, contacts, conversations, chats 😵💫 |
I think you can nix "Contacts". It's not in line with anyone's notion of contacts on a mobile device. I kind of like "Messages", since it matches other chat/messaging apps. Could do a segmented button or up top or a tabbed layout to filter by channels or DMs. |
Contacts should be called contacts |
Could you please elaborate? There's clearly confusion about this screen's function causing it to be referenced by multiple names both in the source/comments and Discord DMs. Are you saying the screen should be called "Contacts" or that the term "Contacts" should be reserved for something else? If I understand correctly, this screen is called "Messages" on iOS. |
}, | ||
channels = channels, | ||
onNodeChipClick = { | ||
if (it.contactKey.contains("!")) { |
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.
should be starts with.
You know someone is going to have a channel containing a !.
They'll probably try and use it to start too, but I feel like that is a reasonable thing to require not to happen
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.
looks fine a few minor comments.
I'll test it now
onNodeChipClick = { | ||
if (it.contactKey.contains("!")) { | ||
// if it's a node, look up the nodeNum including the ! | ||
val nodeKey = it.contactKey.substring(1) |
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.
I prefer explicitly stating what it splits on, even when it's default
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.
- swapping between conversations and nodes results in 2 main bars for a few seconds
- lots of lag switching between tabs (may not be this MR, but I don't feel like it was this bad 48 hours ago)
- Map/ settings / connections should have a second line / blank space left so the Meshtastic text doesn't shift around as you change tabs
This isn't ready for merge yet
Agree - this interacts poorly with the global scaffold and app bar, i might put this on ice until we get that sorted. |
The contacts screen has been refactored to use a list/detail pattern. The
ContactsScreen
composable has been replaced withContacts
and now manages the display of both the list of conversations and the detail view for a selected conversation usingNavigableListDetailPaneScaffold
.The
ContactsRoutes.Contacts
route has been removed andContactsRoutes.Messages
is now the entry point for the contacts graph. TheMessageScreen
is now displayed in the detail pane.The
MainAppBar
composable has been renamed toGlobalAppBar
and the previously privateMainAppBar
component is now public.Deep link URI for messages has been updated to use query parameters.