-
Notifications
You must be signed in to change notification settings - Fork 739
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
Adds Space List Bottom Sheet #6749
Conversation
…-list-modal # Conflicts: # vector/src/main/java/im/vector/app/features/home/NewHomeDetailFragment.kt
…re/eric/space-list-modal # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListFragment.kt
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, but with some comments
changelog.d/6749.feature
Outdated
@@ -0,0 +1 @@ | |||
Adds space list bottom sheet for new app layout |
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.
do we need to provide changelog for features which are not accessible yet?
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.
we should them but they should be .wip
holder.name.text = holder.view.context.getString(R.string.all_chats) | ||
holder.root.isChecked = selected | ||
holder.root.context.resources | ||
holder.avatar.background = ContextCompat.getDrawable(holder.view.context, R.drawable.new_space_home_background) |
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.
isn't it better to set it in xml?
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.
No because we use the same XML for NewSpaceSummaryItem
where we don't want to have this background
holder.root.context.resources | ||
holder.avatar.background = ContextCompat.getDrawable(holder.view.context, R.drawable.new_space_home_background) | ||
holder.avatar.backgroundTintList = ColorStateList.valueOf( | ||
ColorUtils.setAlphaComponent(ThemeUtils.getColor(holder.view.context, R.attr.vctr_content_tertiary), (255 * 0.3).toInt())) |
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.
better to move to xml resource, both declaration of color state and setting it as backgroundTintList
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.
Same as above
@@ -337,7 +338,8 @@ class NewHomeDetailFragment @Inject constructor( | |||
if (fragmentToShow == null) { | |||
when (tab) { | |||
is HomeTab.RoomList -> { | |||
add(R.id.roomListContainer, HomeRoomListFragment::class.java, null, fragmentTag) | |||
val params = RoomListParams(tab.displayMode) |
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.
we don't need any param for HomeRoomListFragment
changelog.d/6749.feature
Outdated
@@ -0,0 +1 @@ | |||
Adds space list bottom sheet for new app layout |
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.
I don't think we need to provide changelogs for features, which are not accessible for users 🤔
# Conflicts: # vector/src/main/res/layout/fragment_room_list.xml
…-list-modal # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListFragment.kt
…-list-modal # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/list/home/HomeRoomListFragment.kt
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.
LGTM
android:layout_gravity="bottom|end" | ||
android:layout_marginEnd="16dp" | ||
android:layout_marginBottom="16dp" | ||
android:accessibilityTraversalBefore="@id/roomListView" |
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.
⚠️ AttributeaccessibilityTraversalBefore
is only used in API level 22 and higher (current min is 21)⚠️ AttributeaccessibilityTraversalBefore
is only used in API level 22 and higher (current min is 21)
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_gravity="bottom|end" | ||
android:layout_marginEnd="16dp" | ||
android:layout_marginBottom="16dp" | ||
android:accessibilityTraversalBefore="@id/roomListView" |
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.
⚠️ AttributeaccessibilityTraversalBefore
is only used in API level 22 and higher (current min is 21)⚠️ AttributeaccessibilityTraversalBefore
is only used in API level 22 and higher (current min is 21)
android:background="@drawable/bg_space_item" | ||
android:clickable="true" | ||
android:focusable="true" | ||
android:foreground="?attr/selectableItemBackground" |
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.
⚠️ Attributeandroid:foreground
has no effect on API levels lower than 23 (current min is 21)⚠️ Attributeandroid:foreground
has no effect on API levels lower than 23 (current min is 21)
android:background="@drawable/bg_space_item" | ||
android:clickable="true" | ||
android:focusable="true" | ||
android:foreground="?attr/selectableItemBackground" |
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.
⚠️ Attributeandroid:foreground
has no effect on API levels lower than 23 (current min is 21)⚠️ Attributeandroid:foreground
has no effect on API levels lower than 23 (current min is 21)
SonarCloud Quality Gate failed. |
Type of change
Content
Adds Space List Bottom Sheet
Motivation and context
Closes #6499
Not included in this PR:
Known bugs to be addressed separately:
Screenshots / GIFs
Video:
spaceswitching2.mp4
Tests
There is a known bug where the bottom sheet doesn't always measure properly. This will be addressed in TBA
Tested devices
Checklist