status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.91k stars 984 forks source link

Chat screen user details new UI #14315

Closed OmarBasem closed 1 year ago

OmarBasem commented 2 years ago

An issue to track the creation of chat screen user details displayed at the top of the messages list inside the chat screen.

Designs.

OmarBasem commented 2 years ago

@J-Son89 I think you were working on a similar design for communities. Would you like to pick this one up to create a reusable component for both?

J-Son89 commented 2 years ago

@OmarBasem, looks pretty similar alright. I think it's best if we create another story for me to abstract what I created into a reusable component and then use this one to implement this with designs? WDYT?

OmarBasem commented 2 years ago

@J-Son89 Can you link the other issue?

J-Son89 commented 2 years ago

Yep, it's this one here: https://github.com/status-im/status-mobile/issues/14320

J-Son89 commented 2 years ago

@jo-mut, as discussed in my p.r. This story is for a generic component to match the designs for communities overview page👍

OmarBasem commented 2 years ago

@J-Son89 I see you already created an issue for that. You can finish the issue, then I will start working on this one. Thank you!

OmarBasem commented 1 year ago

I have tried implementing the new chat header and animations, but there are a few problems with it.

So first of, I have implemented an animated-header-list component in quo2.components.animated-header-flatlist.view to be used from communites screen, messages screen and other screens that require this kind of animation.

Later on, I realized this component won't work for the messages screen as we are using an inverted list. So I tried implementing another one customized for the messages screen, but there are still some issues:

  1. This design shows that the messages list starts from the top. This wouldn't be possible as we are using an inverted list for messages. It starts from the bottom and grows upwards.

  2. Animations: Since we are using an inverted list, thus the header is actually the footer, animating the "header" that way may not be possible. To animate the header we need to know the starting and ending scroll position (for example: from 0 until the header-height). But since what we should actually animate is the footer, we need to precisely know the height of the content of the messages list, which is very dynamic (messages can be added, deleted, reactions can be added, deleted, and more).

  3. Another point, is when there is a chat with 100 messages, the FlatList component won't render all items at the same time, more will be rendered as the user scrolls up, and depending on the device, this can sometimes take a couple of seconds. So as we are scrolling up and about to reach the end of the list, are we going to start animating, or are there more messages to render?

If anybody wants to try with this issue feel free to do so. Otherwise if the above issues are valid then probably we would need some redesign.

cc @cammellos @flexsurfer @status-im/mobile-devs

cammellos commented 1 year ago

@OmarBasem thanks for spotting this issues, this is definitely something that's good to catch early in the development stage.

I think this looks like something that needs to be looked at by designers.

1) Yes, that would be an exception on how we display messages, I believe whatsapp does something similar (empty chat starts at the top, and fill in top to bottom, until screen is filled), while discord puts the "intro" above the input in an empty chat. Given the limitations of FlatList, I take that it would be best to keep the discord behavior as you pointed out.

Regarding 2/3, yes, this animation is only displayed when you reach the "top" of the chat, i.e you have no more messages to load, since you will rarely see this animation on a chat you use often (you will not be scrolling back to the beginning of history), but you might see it on a new chat, though only if we start from the first unread message, which we currently don't since that's problematic with flat list.

I'd say that we should hold on for this one until the flat list replacement is ready, and we should probably accommodate something different for now (just keeping the static header seems like the best option for us)

What do you think @pedro-et @John-44 ?

John-44 commented 1 year ago

@cammellos I think this is something for @pedro-et to look at, however @pedro-et is currently off sick so his reply here will probably be delayed a few days until he is better.

@OmarBasem thanks for flagging this up!

alwx commented 1 year ago

Might be weird but actually I was working on this issue: https://github.com/status-im/status-mobile/pull/15204 And since I thought it's also about all the details (kinda makes no sense to just animate the top bar without working on all the details — it's literally just one component), I implemented it and animated it as well.

It's not ready yet (here is the PR: https://github.com/status-im/status-mobile/pull/15204) but that's how it currently looks in that branch:

https://user-images.githubusercontent.com/911127/222123864-1406f27a-8dbf-486f-939d-3b32fb74b237.mov

alwx commented 1 year ago

There are still some minor issues to fix but I can either continue further with that or just stop working if @OmarBasem wants to pick it all up and continue. Sorry, I think it was a bit of a confusion because there are multiple issues created for literally one component.

cammellos commented 1 year ago

Ah good work @alwx! Ok, let's close maybe the other issue if you could and assign this one to you?

We can still have a discussion taking into account the progress made so far, just wondering how it looks like on an empty chat? (i.e is the banner on top and messages are displayed top to bottom etc)

alwx commented 1 year ago

@OmarBasem are you fine with that?

OmarBasem commented 1 year ago

multiple issues created for literally one component.

The 2 issues are slightly different, this one was supposed to be for the details part, and the other one for the animations, but they can be merged into one issue I think.

@alwx I see you are using the animated-header-list component. Which means the list is not inverted, right?

John-44 commented 1 year ago

@alwx do you think you might be able implement the design as-is? If so great news (and then @pedro-et doesn't need to spend time looking into this), or might this still need some design attention?

alwx commented 1 year ago

@OmarBasem yep, it's not — I saw that you made it work that way that it's literally a FlatList that contains another (reverted) FlatList as its element. I modified the component though — and I'm currently playing with animations and trying to make them work fine with a reverted FlatList as well. I think you might be right that it all might become problematic when there are a lot of messages in the list and when we need to scroll for a while before we reach the end — except this issue, I don't feel it's too hard to make it work right.

@John-44 if you have any ideas about scrolling and that issue with messages being loaded once we scroll (which leads to problems with determining if the end of the list is truly the end), feel free to tell. The easiest solution might be to just not show the user details on top if the list is too long.

OmarBasem commented 1 year ago

@alwx the animated component itself is a list, and the main-component prop can be anything, which in this case a list also.

You can keep working on it, and let us know if you reach something.

alwx commented 1 year ago

A small update: Managed to update it all to work with a reversed list (see last commit) but there is another problem with this approach — when there is a list inside a list then .scrollTo doesn't work properly. Currently trying to figure what to do with that.

pedro-et commented 1 year ago

@alwx and @OmarBasem glad to see you're making progress! Looking forward to seeing you crack it in no time :)

alwx commented 1 year ago

Managed to make it all work! 💪

https://user-images.githubusercontent.com/911127/223729823-4db8c1c7-a016-48a4-9481-454cc3e7f5a7.mov

All the changes are now available in this PR: https://github.com/status-im/status-mobile/pull/15204

OmarBasem commented 1 year ago

Well done @alwx giving this a go. I have done quick testing and commented on the PR some issues that I found: https://github.com/status-im/status-mobile/pull/15204#issuecomment-1460233314

alwx commented 1 year ago

@OmarBasem thanks for checking. Yes, there are still some issues, hopefully mostly minor ones (except that the blur doesn't work on Android, @flexsurfer tried it with other components as well)

John-44 commented 1 year ago

@alwx super, well done, great to see :-) Thanks for persevering, much appreciated 🙏