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

Chat screen user details new UI #14315

Closed
OmarBasem opened this issue Nov 10, 2022 · 23 comments · Fixed by #15204
Closed

Chat screen user details new UI #14315

OmarBasem opened this issue Nov 10, 2022 · 23 comments · Fixed by #15204
Assignees
Labels

Comments

@OmarBasem
Copy link
Member

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 OmarBasem added the feature feature requests label Nov 10, 2022
@OmarBasem OmarBasem self-assigned this Nov 10, 2022
@OmarBasem OmarBasem removed their assignment Nov 10, 2022
@OmarBasem
Copy link
Member Author

@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
Copy link
Member

J-Son89 commented Nov 10, 2022

@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
Copy link
Member Author

@J-Son89 Can you link the other issue?

@J-Son89
Copy link
Member

J-Son89 commented Nov 10, 2022

Yep, it's this one here: #14320

@J-Son89
Copy link
Member

J-Son89 commented Nov 10, 2022

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

@OmarBasem
Copy link
Member Author

@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!

@churik churik added this to the 1.21.0-rc.1 milestone Dec 2, 2022
@OmarBasem OmarBasem removed the blocked label Dec 29, 2022
@cammellos cammellos modified the milestones: 1.21.0-rc.1, 1.21.0-rc.2 Jan 20, 2023
@OmarBasem
Copy link
Member Author

OmarBasem commented Mar 1, 2023

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
Copy link
Member

@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
Copy link

John-44 commented Mar 1, 2023

@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
Copy link
Contributor

alwx commented Mar 1, 2023

Might be weird but actually I was working on this issue: #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: #15204) but that's how it currently looks in that branch:

Screen.Recording.2023-03-01.at.12.13.45.mov

@alwx
Copy link
Contributor

alwx commented Mar 1, 2023

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
Copy link
Member

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
Copy link
Contributor

alwx commented Mar 1, 2023

@OmarBasem are you fine with that?

@alwx alwx self-assigned this Mar 1, 2023
@OmarBasem
Copy link
Member Author

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
Copy link

John-44 commented Mar 1, 2023

@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
Copy link
Contributor

alwx commented Mar 1, 2023

@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
Copy link
Member Author

OmarBasem commented Mar 1, 2023

@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
Copy link
Contributor

alwx commented Mar 2, 2023

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
Copy link

pedro-et commented Mar 2, 2023

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

@alwx
Copy link
Contributor

alwx commented Mar 8, 2023

Managed to make it all work! 💪

Screen.Recording.2023-03-08.at.14.44.13.mov

All the changes are now available in this PR: #15204

@OmarBasem
Copy link
Member Author

Well done @alwx giving this a go. I have done quick testing and commented on the PR some issues that I found: #15204 (comment)

@alwx
Copy link
Contributor

alwx commented Mar 8, 2023

@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
Copy link

John-44 commented Mar 8, 2023

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

@cammellos cammellos modified the milestones: 1.22.0, 1.23.0 Mar 27, 2023
@OmarBasem OmarBasem removed their assignment May 8, 2023
@churik churik modified the milestones: 1.23.0 - Alpha, 1.24.0 - Alpha May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants