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

feat: add unseen messages banner and scroll to first unseen functionality #209

Merged
merged 15 commits into from Aug 6, 2022

Conversation

felixgabler
Copy link
Collaborator

@felixgabler felixgabler commented Jan 31, 2022

What does it do?

Add an unread messages banner and the option to scroll to the first unread message on chat open.

Screenshot 2022-01-31 at 14 42 19

Why is it needed?

Users should have a way of being brought directly to the content they need to see most urgently. In many cases this is the first unread message.

How to test it?

In the example app, I have set scrollToUnseenOnOpen so it should immediately scroll to the first message.

Related issues/PRs

Closes #179 and closes #213

This is built on top of #208 and should not be merged before it. Second part of #179.

Open Questions / TODO

  • Should there be an option to disable the banner too?
  • Designer: I need designs for the banner and then I can also make them configurable in the ChatTheme
  • I will write some tests for my changes to the 'util' function
  • The unseen message calculation currently only works if the status is set correctly for each message. I wonder whether the statuses for received messages are even really relevant and can be used like this. Wouldn't it make more sense to have a seenBy array on messages to check for the first message which is not marked as seen by the current user?
  • Scroll to bottom indicator (separate issue, tackle later)

@demchenkoalex
Copy link
Member

Will ask @dariakhimych to create some designs :) Floating top button might a good addition too, as long as we can calculate the offset we need to scroll to.

About seenBy should it be an array? Can we store there like a last seen message id and then iterate though messages until that id and mark them as seen? Or I didn't get the concept..

@felixgabler
Copy link
Collaborator Author

Cool!

To me, there are two concepts one could use to improve the seen by:

  1. Either a chat level array/map that tells us for every user what the last message was they read.

  2. Or an array on each message that contains the room members' IDs who have seen it.

I actually like 1. now that you mentioned it because one can also compute 2. quite easily for most use cases.

The only drawback of 1. is that we can "forget" about older messages that we have not read.
As an example, let's say we have 100 new messages and open the chat without scroll to top. Then, we look at the newest 10 but ignore the other ones above. This would set the "last message seen" for my user to the newest message and it would be as if I have seen them all.

On the other hand, I might even prefer this over the other way that e.g. Slack uses where you actually have to look at every message or the top scroll button does not disappear (or you can dismiss the top scroll button). But that is perhaps just me.

We would just have to make sure that the unread message banner location is calculated on initialization and does not change later because otherwise it would go away immediately when the "last message seen" array changes for my user and points to the newest message that I now have seen.
So we would have to make sure to store that state a bit longer.

But 2. would surely also work and be more like Slack. I'm not super satisfied with how the statuses are then not the whole source of truth because "seenByAll" would be computed based on whether the array is full. Also, this way would require write permissions on the message which is basically a no-go actually.

What do you think?

@felixgabler
Copy link
Collaborator Author

I tested out scroll_to_id and switched to using it. This will enable more functionality in the future. For example, we can use it to scroll to a reply. With my previous way, we'd have to use many GlobalKeys that are more resource intensive.

Also, now you can specify a firstUnseenMessageID that gives users more control about where to show the unseen message banner and where to scroll to. I also export the scrolling functions as scrollToFirstUnseen() and scrollToMessage(String id) that can be very useful in the future.

@felixgabler
Copy link
Collaborator Author

Hey! No pressure but are there any updates in terms of the design?

@demchenkoalex
Copy link
Member

Yeah, will cycle back on this one.

@demchenkoalex
Copy link
Member

Icon: icon-arrow.zip

Screenshot 2022-08-02 at 19 18 37

in Telegram I noticed chat opens already on the "unread messages" banner, so this component on the screenshot is theoretically something else, scroll to bottom, when you scroll a bit of the chat, but if you want a scroll to first unread, you can just reverse the icon to point top.

@felixgabler felixgabler marked this pull request as ready for review August 3, 2022 15:31
@felixgabler
Copy link
Collaborator Author

@demchenkoalex I'd be in favour of adding the scroll to bottom indicator in a separate PR. If you agree, let us create a new issue for it with the icon attached.

Please consider that the scrollController handling has become a bit weird now since we need to use an AutoScrollController. Specifying the scrollController option in chat will disable auto scrolling now. Do you have a better solution?

Also, I tried to make the distances configurable too but it is quite the pain right now because distances between messages are added in many different places (margins, spacer objects, etc). We should rework this when we go over the Themes.

@demchenkoalex
Copy link
Member

I'd be in favour of adding the scroll to bottom indicator in a separate PR

Yes. I have added icon to the project on main to not lose it.

Please consider that the scrollController handling has become a bit weird

There is actually another PR opened which will allow us to move from calculating diffs and animated list view to the normal list view, and scrollController might be easier to work with?

distances between messages are added in many different places

This was a limitation I faced when using animated list view. If the PR I mentioned above will work as I expect it to work, we will be able to unify all distances, as well as give option to render time inside the bubble.

@felixgabler
Copy link
Collaborator Author

felixgabler commented Aug 4, 2022

I looked at the PRs and unfortunately, they will not help in this regard. It's about which scroll controller we use if one is provided. We could either require users to pass in an AutoScrollController (and make sure we export it) or go the route of disabling the scroll to index if scrollController is provided. Personally, I'd be in favour of the first option.

Another small thing to decide: Should we jump to the latest unread or scroll like we do here? One could override it right now by setting the scrollTo duration to zero.

@demchenkoalex
Copy link
Member

I think we jump, but it can be configurable. As for the passing controller - users will need to always manually pass it? Less params to pass = better IMO, no way to set it as default one without users to explicitly pass it? or if they want to override it just say that they should use the one we export, similarly to the InputTextFieldController we have exported?

@felixgabler
Copy link
Collaborator Author

I think we jump, but it can be configurable. As for the passing controller - users will need to always manually pass it? Less params to pass = better IMO, no way to set it as default one without users to explicitly pass it? or if they want to override it just say that they should use the one we export, similarly to the InputTextFieldController we have exported?

With "require", I actually just meant that if they provide one it has to be an AutoScrollController instead of a regular ScrollController like they can use now. So yes, it would be as you described

@demchenkoalex
Copy link
Member

This is fine then 👍

@demchenkoalex demchenkoalex merged commit 36e1dd4 into flyerhq:main Aug 6, 2022
@demchenkoalex
Copy link
Member

Thanks for the PR!

@felixgabler felixgabler deleted the fg/scroll_to_unseen branch November 11, 2022 13:05
@dan91 dan91 mentioned this pull request Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scroll chat to a particular message Open at first unread message
2 participants