Skip to content
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: move to folder [WPB-14627] #3787

Merged
merged 20 commits into from
Jan 14, 2025
Merged

feat: move to folder [WPB-14627] #3787

merged 20 commits into from
Jan 14, 2025

Conversation

Garzas
Copy link
Contributor

@Garzas Garzas commented Jan 7, 2025

TaskWPB-14627 [Android] Conversation folder - Move to folder


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

  • move conversation to folder screen
  • conversation bottom sheet option to move conversation to folder from conversation list and from conversation details screen (for other user profile screen is currently disable, will need additional work on kalium to get conversation details)
  • snackbars with information about success and failure

Attachments (Optional)

screen-20250107-095349.mp4

@Garzas Garzas self-assigned this Jan 7, 2025
@echoes-hq echoes-hq bot added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Jan 7, 2025
@Garzas Garzas marked this pull request as ready for review January 7, 2025 12:21
}

@Composable
private fun Content(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I would prefer to have "content" composables without injections nor viewmodels so that previews can be created more easier, would it be complicated to move these viewmodel injections up to the ConversationFoldersScreen? 🤔

Comment on lines 71 to 75
val args = remember {
navigator.navController.currentBackStackEntry?.let {
ConversationFoldersScreenDestination.argsFrom(it)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: for destination composables you can get args more easily by just adding it as a parameter:

@Composable
fun ConversationFoldersScreen(
    args: ConversationFoldersNavArgs,
    navigator: Navigator,
    resultNavigator: ResultBackNavigator<ConversationFoldersNavBackArgs>,
)

val resources = LocalContext.current.resources
val context = LocalContext.current

LaunchedEffect(Unit) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use lifecycle.repeatOnLifecycle() here to collect only when app is in foreground?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is unnecessary because even if user goes to background after triggering action it should close select folder screen either way

private val moveConversationToFolder: MoveConversationToFolderUseCase,
) : MoveConversationToFolderVM, ViewModel() {

var state: MoveConversationToFolderState by mutableStateOf(MoveConversationToFolderState())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be private? Or keep it public with private get() and then actionableState() is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, nice catch 👏

@Garzas Garzas requested a review from saleniuk January 13, 2025 09:51
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 29.03226% with 44 lines in your changes missing coverage. Please review.

Project coverage is 45.77%. Comparing base (2f1c38d) to head (6ba52c0).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...conversations/folder/MoveConversationToFolderVM.kt 0.00% 34 Missing ⚠️
...home/conversations/folder/ConversationFoldersVM.kt 0.00% 8 Missing ⚠️
...conversations/folder/ConversationFoldersNavArgs.kt 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3787      +/-   ##
===========================================
- Coverage    45.85%   45.77%   -0.09%     
===========================================
  Files          483      485       +2     
  Lines        16510    16556      +46     
  Branches      2779     2781       +2     
===========================================
+ Hits          7571     7578       +7     
- Misses        8159     8198      +39     
  Partials       780      780              
Files with missing lines Coverage Δ
...tlin/com/wire/android/mapper/ConversationMapper.kt 68.00% <100.00%> (+0.65%) ⬆️
...tions/details/GroupConversationDetailsViewModel.kt 67.37% <100.00%> (+0.17%) ⬆️
...home/conversationslist/model/ConversationFolder.kt 42.85% <ø> (ø)
...i/home/conversationslist/model/ConversationItem.kt 56.09% <100.00%> (+1.66%) ⬆️
...erprofile/other/OtherUserProfileScreenViewModel.kt 59.70% <100.00%> (+0.20%) ⬆️
...conversations/folder/ConversationFoldersNavArgs.kt 0.00% <0.00%> (ø)
...home/conversations/folder/ConversationFoldersVM.kt 0.00% <0.00%> (ø)
...conversations/folder/MoveConversationToFolderVM.kt 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f1c38d...6ba52c0. Read the comment docs.

Copy link
Contributor

Built wire-android-staging-compat-pr-3787.apk is available for download

Copy link
Contributor

Built wire-android-dev-debug-pr-3787.apk is available for download

Copy link
Contributor

Built wire-android-staging-compat-pr-3787.apk is available for download

Copy link
Contributor

Built wire-android-dev-debug-pr-3787.apk is available for download

@Garzas Garzas added this pull request to the merge queue Jan 14, 2025
Merged via the queue into develop with commit 6ab51e6 Jan 14, 2025
13 checks passed
@Garzas Garzas deleted the feat/move-to-folder branch January 14, 2025 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants