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

New App Layout: Adds New Chat Bottom Sheet #6801

Merged
merged 8 commits into from
Aug 19, 2022

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Aug 10, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Motivation and context

Closes #6717

This is the new way we differentiate between a new DM or a new create room chat when using the FAB on the main screen

Screenshots / GIFs

image

Tests

  • Enable the New App Layout Feature Flag
  • Click the primary FAB
  • See bottom sheet
  • Ensure both options work

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 12

Checklist

@ericdecanini ericdecanini added the PR-Small PR with less than 20 updated lines label Aug 10, 2022
@ericdecanini ericdecanini requested review from fedrunov and a team August 10, 2022 15:44
android:paddingHorizontal="16dp"
android:paddingVertical="16dp"
android:text="@string/start_chat"
android:textAppearance="@style/TextAppearance.Vector.Body"
Copy link
Contributor

Choose a reason for hiding this comment

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

you apply both style and textAppearance`. We should only apply style which fits by text size and override other params if necessary

android:paddingHorizontal="16dp"
android:paddingVertical="16dp"
android:text="@string/create_room"
android:textAppearance="@style/TextAppearance.Vector.Body"
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

}

binding.createRoom.setOnClickListener {
navigator.openCreateRoom(requireActivity(), "")
Copy link
Contributor

Choose a reason for hiding this comment

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

initialName param of openCreateRoom has default value, so you can just skip it

@ericdecanini ericdecanini requested review from fedrunov and removed request for fedrunov August 17, 2022 11:37
@ericdecanini ericdecanini requested a review from fedrunov August 17, 2022 19:42
@ericdecanini ericdecanini requested a review from mnaturel August 17, 2022 20:32
Copy link
Contributor

@fedrunov fedrunov left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ericdecanini ericdecanini requested review from onurays and removed request for mnaturel August 19, 2022 12:30
@@ -166,7 +168,7 @@ class HomeRoomListFragment @Inject constructor(
showFABs()

views.newLayoutCreateChatButton.setOnClickListener {
// Click action for create chat modal goes here (Issue #6717)
newChatBottomSheet.show(requireActivity().supportFragmentManager, NewChatBottomSheet.TAG)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Wdyt about creating the bottom sheet instance here instead of a class variable?

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 wouldn't prefer doing it this way because we're creating new instances of the bottom sheet every time and adding it to memory

Copy link
Contributor

@onurays onurays left a comment

Choose a reason for hiding this comment

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

LGTM.

@ericdecanini ericdecanini merged commit 11b4ea5 into develop Aug 19, 2022
@ericdecanini ericdecanini deleted the feature/eric/new-chat-bottom-sheet branch August 19, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Small PR with less than 20 updated lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Space Switching: New Chat Bottom Sheet
3 participants