-
Notifications
You must be signed in to change notification settings - Fork 23
feat: add new conversation filters - WPB-18707 #3327
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
feat: add new conversation filters - WPB-18707 #3327
Conversation
Test Results4 647 tests 4 619 ✅ 7m 29s ⏱️ Results for commit a39f00c. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 4664 Passed, 28 Skipped, 3m 50.48s Total Time |
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 for the contribution @emil-wire, it will be definitely useful!
2 things:
- I have to kill the app for the flag to take effect
- I am surprised the filter is not applied at the SyncEngine level. We usually add the predicates in ZMConversationListDirectory.
- would be cool to add the draft as well
...List/ListContent/ConversationListContentController/ViewModel/ConversationListViewModel.swift
Outdated
Show resolved
Hide resolved
@netbe I added drafts, moved the predicates to the right place and hopefully fixed the flag. |
...re-iOS/Sources/UserInterface/ConversationList/Container/ConversationListViewController.swift
Outdated
Show resolved
Hide resolved
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.
Great work! Thank you for contributing! Before merging I think we should either:
- remove the observation of the developer flag
- find a way to make it more granular (I expect KVO) on the UserDefaults object
...re-iOS/Sources/UserInterface/ConversationList/Container/ConversationListViewController.swift
Outdated
Show resolved
Hide resolved
wire-ios/Wire-iOS/Sources/UserInterface/MainController/ZClientViewController.swift
Outdated
Show resolved
Hide resolved
...ersationList/ConversationListViewModel/ConversationListViewModelFilterIntegrationTests.swift
Outdated
Show resolved
Hide resolved
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 good, just fix the remained comments tests fixed.
The UI snapshots need to be recorded too with:
IOS_VERSION='18.0'
IPHONE_MODEL='iPhone 14'
...re-iOS/Sources/UserInterface/ConversationList/Container/ConversationListViewController.swift
Outdated
Show resolved
Hide resolved
...ersationList/ConversationListViewModel/ConversationListViewModelFilterIntegrationTests.swift
Show resolved
Hide resolved
@netbe @samwyndham I've used KVO instead as suggested and fixed up the tests. Added some snapshots using the correct device/os combo too |
…d-filter-for-unread-messages-WPB-18707 | Base: 43bf1fa
WireUI/Sources/WireMainNavigationUI/Containers/MainSplitViewController.swift
Show resolved
Hide resolved
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.
Great work! I'm excited to use this!
Issue
This is my attempt at adding some additional filters to the app. I've tried to stick to the general implementation of the other filter items that already exist. The filters cover unread, mentions and replies. They're hidden behind a feature flag because the design isn't finalized yet. Since this is my first contribution, I'd appreciate all the feedback I can get, especially what tests I should write.
Testing
Open the app, tap the conversation filters and give it a spin
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: