-
Notifications
You must be signed in to change notification settings - Fork 552
[NEW] Add bottomSheet in Chatrooms #1929
base: develop
Are you sure you want to change the base?
Conversation
Hey @shubhsherl, thanks a lot for this PR (I'm happy that you did it as pointed on you comment on the issue). Make sure to remove the [WIP] when it is done, so we can review it. |
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.
Just leaving some comments here.
class RoomViewHolder(itemView: View, private val listener: (RoomUiModel) -> Unit, actionsListener: ActionsListener) : | ||
ViewHolder<RoomItemHolder>(itemView, actionsListener) { | ||
|
||
init { |
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.
For a better coding style this initializer block comes after the property declaration.
https://kotlinlang.org/docs/reference/coding-conventions.html
presenter.toggleFavoriteChatRoom(this.id, this.favorite==true) | ||
} | ||
R.id.action_leave_room-> { | ||
|
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.
Let us know if you are planning to add a function to leave the room here.
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.
Yaa, I am working on it.
this.unread.ifNotNullNorEmpty{ presenter.markRoomAsRead(this.id) } | ||
} | ||
R.id.action_hide_room->{ | ||
|
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.
And to hide the room too.
fun isActionsEnabled(): Boolean | ||
fun onActionSelected(item: MenuItem, room: RoomUiModel) | ||
} | ||
|
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.
Remove this empty line.
retryIO("favorite($roomId, $isFavorite)") { | ||
client.favorite(roomId, !isFavorite) | ||
} | ||
// view.showFavoriteIcon(!isFavorite) |
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 need to show/hide that icon after user interaction.
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.
The bottom sheet will disappear after toggle, and next time it will fetch from toggled data. So i guess, no need of showFavoriteIcon
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.
But are this PR adding these features from the chat list or inside a chat room?
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.
From chatList, I was taking reference of code implemented in ChatRoom, that's why the function is there. I will remove that.
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! Don't remove that, please! Give the user the possibility to star/unstar a chat inside a chat room (like we do for the web for instance).
You should leave the current feature as it is because removing that will force the user to go back to the chat list to do these actions every time.
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, I am not removing the current feature.
These functions are implemented in Chatlist( not chatRoom). So it doesn't change the feature of Chatroom.
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.
Ah, ok 😃 Then we shouldn't put this function neither on ChatRoomsPresenter.kt nor ChatRoomPresenter. We should put it in only one place to be used in the Chat List and ChatRoom view.
app/src/main/java/chat/rocket/android/chatrooms/presentation/ChatRoomsView.kt
Outdated
Show resolved
Hide resolved
@filipedelimabrito Work Done, I have added a function in Rocket.Chat.Kotlin.SDK for markasunread. |
@RocketChat/android
Closes #1055
Add bottom sheet for hide, Leave, Favorite, MarkAsRead chatrooms