feat: Wire drive filter (WPB-20721)#4612
feat: Wire drive filter (WPB-20721)#4612ohassine wants to merge 24 commits intoepic-filter-sort-search-in-shared-drivefrom
Conversation
# Conflicts: # app/src/main/kotlin/com/wire/android/navigation/MainNavHost.kt # app/src/main/kotlin/com/wire/android/navigation/WireMainNavGraph.kt # build-logic/plugins/src/main/kotlin/AndroidCoordinates.kt # core/ui-common/src/main/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetState.kt # features/cells/src/main/java/com/wire/android/feature/cells/ui/ConversationFilesScreen.kt # features/cells/src/main/java/com/wire/android/feature/cells/ui/ConversationFilesWithSlideInTransitionScreen.kt
Ups 🫰🟨This PR is too big. Please try to break it up into smaller PRs. |
| FilterChip( | ||
| modifier = modifier.wrapContentSize(), | ||
| onClick = { onSelectChip(label) }, | ||
| onClick = { onClick(label) }, |
There was a problem hiding this comment.
Minor comment: "label" must be always passed by the WireFilterChip caller, no need to return it back in onClick(label). Could be just onClick().
There was a problem hiding this comment.
yes I will remove it after removing FilterBottomSheet
| ), | ||
| ).map { pagingData: PagingData<Node> -> | ||
| pagingData.map { node: Node -> | ||
| if (uiState.value.availableOwners.isEmpty()) { |
There was a problem hiding this comment.
I do not understand how this is supposed to work. Why do we launch a new coroutine here (and only if not empty:
- It does not handle duplicates. If there are 10 files from same user it will launch 10 coroutines to get user info
- It stops adding new users if availableUsers already have some values (?). There will be no filters for users from the second page of files.
I think it's worth to re-think approach to this filter logic.
There was a problem hiding this comment.
Yeah changed this by calling a separate usecase that fetches owners
...om/wire/android/feature/cells/ui/search/filter/bottomsheet/owner/FilterByOwnerBottomSheet.kt
Outdated
Show resolved
Hide resolved
features/cells/src/main/java/com/wire/android/feature/cells/ui/search/SearchUiState.kt
Show resolved
Hide resolved
|
| searchBarHint = stringResource(R.string.search_shared_drive_text_input_hint), | ||
| searchQueryTextState = conversationSearchBarState.searchQueryTextState, | ||
| onActiveChanged = conversationSearchBarState::searchActiveChanged, | ||
| onTap = { |
There was a problem hiding this comment.
Do we really need a separate onTap? Can't it be done using onActiveChanged?
With onTap it will only work when user clicks on the search input, but won't work when user switches focus in a different way, and it would work when using onActiveChanged.
Also, I think that the UX is wrong when user navigates from SearchConversationMessagesScreen and clicks "Search Files" - then ConversationFilesScreen opens with search bar focused, so user has two "back" buttons on right next to each other, and actually needs to tap the search bar again, even though it's already focused, to actually navigate to SearchScreen and be able to use all these filters.
Take a look at the video:
Screen.Recording.2026-03-03.at.15.06.35.mov
| bottomContent: @Composable ColumnScope.() -> Unit = {} | ||
| bottomContent: @Composable ColumnScope.() -> Unit = {}, | ||
| onTap: (() -> Unit)? = null, | ||
| focusManager: FocusManager = LocalFocusManager.current, |
There was a problem hiding this comment.
You don't need to make it a parameter that needs to be passed from the parent and actually you shouldn't as that's the whole point of LocalFocusManager so that you can use it without the need of passing it down.
| isSearchActive: Boolean, | ||
| searchBarHint: String, | ||
| searchQueryTextState: TextFieldState, | ||
| focusRequester: FocusRequester, |
There was a problem hiding this comment.
Is this focusRequester needed to be a parameter? I don't see it being used anywhere outside of this SearchTopBar so it could stay just inside this composable, just as it was before.
| .invokeOnCompletion { onDismiss() } | ||
| } | ||
|
|
||
| ModalBottomSheet( |
There was a problem hiding this comment.
Why not WireModalSheetLayout to keep all our bottom sheets unified?
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| fun FilterByOwnerBottomSheet( |
There was a problem hiding this comment.
All bottom sheets have this weird jumping when user flings and it reaches the end:
Screen_recording_20260303_164541.mp4
It looks like some general ModalBottomSheet issue:https://issuetracker.google.com/issues/285847707
It's happening for all our bottom sheets, it's just more prominent in these ones as they have nested LazyColumn.
Google fixed that in material3 v1.5.0-alpha10, just FYI, save yourself from trying to fix it. 😄
| start = dimensions().spacing12x, | ||
| end = dimensions().spacing12x, | ||
| top = dimensions().spacing8x, | ||
| bottom = dimensions().spacing8x |
There was a problem hiding this comment.
can be: horizontal = dimensions().spacing12x, vertical = dimensions().spacing8x
3 participants
https://wearezeta.atlassian.net/browse/WPB-20721
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Description
Implement filter feature in shared drive
screen-20260223-133714-1771850185637.mp4
Needs releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.