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

Feature: Unread posts indicator #375

Merged
merged 22 commits into from
Feb 13, 2025

Conversation

FelberMartin
Copy link
Collaborator

@FelberMartin FelberMartin commented Feb 2, 2025

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

  • Added unread indicator
  • Added index to paged posts to correctly display the unread indicator

Steps for testing

  • Go to any chat with unread posts
  • See that a visual indicator separates read from unread posts

Screenshots

image

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

@FelberMartin FelberMartin self-assigned this Feb 2, 2025
@FelberMartin FelberMartin marked this pull request as ready for review February 6, 2025 15:07
@FelberMartin FelberMartin added the ready for review This PR can be reviewed label Feb 6, 2025
@FelberMartin FelberMartin marked this pull request as draft February 7, 2025 10:14
@FelberMartin FelberMartin force-pushed the feature/communication/unread-posts-indicator branch from 980a509 to e990bcf Compare February 7, 2025 14:51
@FelberMartin FelberMartin changed the base branch from develop to bugfix/communication/not-increase-unread-counter-in-chat February 7, 2025 14:52
@FelberMartin FelberMartin force-pushed the feature/communication/unread-posts-indicator branch from e990bcf to 84aafb5 Compare February 7, 2025 14:56
Copy link
Contributor

@eylulnc eylulnc left a 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.

Base automatically changed from bugfix/communication/not-increase-unread-counter-in-chat to develop February 13, 2025 08:44
Copy link
Contributor

@julian-wls julian-wls left a 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):

@FelberMartin
Copy link
Collaborator Author

FelberMartin commented Feb 13, 2025

I also experienced the second bug that you mentioned, I think this one is (as you mentioned) server related and also occurs on develop. This bug however could also be mitigated by storing the conversations locally.

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

@FelberMartin FelberMartin added ready to merge This PR can be merged and removed ready for review This PR can be reviewed labels Feb 13, 2025
@FelberMartin FelberMartin marked this pull request as ready for review February 13, 2025 09:46
Copy link
Contributor

@julian-wls julian-wls left a 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.

@FelberMartin FelberMartin merged commit 2356353 into develop Feb 13, 2025
5 checks passed
@FelberMartin FelberMartin deleted the feature/communication/unread-posts-indicator branch February 13, 2025 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants