-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature
: Unread posts indicator
#375
Feature
: Unread posts indicator
#375
Conversation
980a509
to
e990bcf
Compare
e990bcf
to
84aafb5
Compare
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.
Code looks good.
Works as described, I did not encounter any problem other than the one mentioned in the description.
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 just tested it and it works fine most of the time. I got two concerns though:
Regarding the bug you mentioned my opinion would be to release this feature only if the bug is fixed. I think you can merge it now but not release it until we tackled this bug. I experienced this bug a couple of times and found it quite confusing for users.
Another problem I experienced was the following:
x.mp4
I just wanted to mention it here, but I think it is server related as the channels sometimes don't update. When going back to the channel the unread indicator was also still there.
And one comment (Everything else lgtm):
.../informatics/www1/artemis/native_app/feature/metis/conversation/ui/chatlist/MetisChatList.kt
Outdated
Show resolved
Hide resolved
I also experienced the second bug that you mentioned, I think this one is (as you mentioned) server related and also occurs on Overall I agree with you that the resulting behavior of these two bugs are quite disturbing, so I will merge this PR now, but include a feature toggle that deactives the feature for now. When the bugfixes are introduced, we then can also remove the feature toggle again. I also updated #394 to keep track of the mentioned TODOs |
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 agree. Lgtm now.
Problem Description
When navigating to a chat with unread posts, the users had no clear indicator where the new unread posts begin. This PR adds an unread indicator inspired by slack and ls1intum/Artemis#10018
Changes
Steps for testing
Screenshots
Follow-up
Known bug:
When the unread counter in the ConversationOverview increases due to a websocket update (the user is currently in the communication tab and a new post in a chat that is not open is sent), then the unread indicator is not displayed. This is due to the unread counter only being updated in the ConversationOverviewViewmodel. This bug will be tackled with #394