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.

Sign up for GitHub

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

Jump to bottom

fix: bottom sheet state [WPB-9045] #3065

Merged
merged 6 commits into from
Jun 4, 2024
Merged

fix: bottom sheet state [WPB-9045] #3065

merged 6 commits into from
Jun 4, 2024

Conversation

Garzas
Copy link
Contributor

@Garzas Garzas commented Jun 3, 2024 •
edited by github-actions bot
Loading

BugWPB-9045 [Android] Context menu missing when unarchiving a conversation

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?

  • fixed not showing bottom sheet in archive conversations screen
  • removed unused calls and mentions screens
  • added generic SnackBarMessageHandler to show snackbar messages
  • splitted view models to one responsible for incoming calls and conversation list
  • removed duplicated instances of incoming call view model
  • refactored HomeSnackBarMessage to extend SnackBarMessage
  • implemented WireModalSheetLayout2 for easier state management

@Garzas Garzas self-assigned this Jun 3, 2024
@pull-request-size Pull Request Size
Copy link

Ups 🫰🟨

This PR is too big. Please try to break it up into smaller PRs.

@Garzas Garzas requested a review from vitorhugods June 3, 2024 12:03
@AndroidBob
Copy link
Collaborator

Build 5119 failed.

@AndroidBob
Copy link
Collaborator

Build 5120 succeeded.

The build produced the following APK's:

saleniuk
saleniuk approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@saleniuk saleniuk left a comment

Choose a reason for hiding this comment

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

Great job! Just little suggestions 😄

app/src/main/kotlin/com/wire/android/ui/home/conversationslist/ConversationCallListViewModel.kt

@Suppress("MagicNumber", "TooManyFunctions", "LongParameterList")
@HiltViewModel
class ConversationCallListViewModel @Inject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: maybe swap Call and List in the name? ConversationListCallViewModel sounds better to me as it's used in "conversations list" composable and it's purpose is calls, currently it suggest that it's used in "conversation call list", so list of conversation calls? 😄 Also, state is named "ConversationListCallState" so it would match then 😉

app/src/main/kotlin/com/wire/android/ui/home/conversationslist/ConversationRouter.kt Outdated
) {
var currentConversationItem by remember {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: maybe currentSheetConversationItem to emphasise that this mutable state is used for the bottom sheet?

core/ui-common/src/main/kotlin/com/wire/android/ui/common/bottomsheet/WireModalSheetLayout.kt

@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun WireModalSheetLayout2(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: maybe mark previous one as deprecated so that it's clear that this one is favourable 😄

@Garzas Garzas requested a review from alexandreferris June 4, 2024 10:06
@AndroidBob
Copy link
Collaborator

Build 5141 succeeded.

The build produced the following APK's:

vitorhugods
vitorhugods approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@vitorhugods vitorhugods left a comment

Choose a reason for hiding this comment

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

Awesome 😎

Just a small thingy on top of @saleniuk's comments

...src/main/kotlin/com/wire/android/ui/home/conversations/call/ConversationListCallViewModel.kt Outdated
Comment on lines 104 to 107
!(
conversationDetailsResult.conversationDetails is ConversationDetails.Group
&& !(conversationDetailsResult.conversationDetails as ConversationDetails.Group).isSelfUserMember
)
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit hard to read.
Could you please extract to more readable vals before putting into the if?

Like

val isGroupConversation = ...

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Garzas Garzas enabled auto-merge June 4, 2024 20:15
@Garzas Garzas added this pull request to the merge queue Jun 4, 2024
Merged via the queue into develop with commit 060e71e Jun 4, 2024
10 of 11 checks passed
@Garzas Garzas deleted the fix/bottom-sheet-state branch June 4, 2024 20:42
@AndroidBob
Copy link
Collaborator

Build 5157 succeeded.

The build produced the following APK's:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saleniuk saleniuk saleniuk approved these changes

@vitorhugods vitorhugods vitorhugods approved these changes

@borichellow borichellow Awaiting requested review from borichellow

@ohassine ohassine Awaiting requested review from ohassine

@alexandreferris alexandreferris Awaiting requested review from alexandreferris

Assignees

@Garzas Garzas

Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Garzas @AndroidBob @vitorhugods @saleniuk