-
Notifications
You must be signed in to change notification settings - Fork 984
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
[#7454] Add basic copy to public chat empty screen state #7663
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (94)
|
@bitsikka checking the builds now |
I had not accounted for the fact that private group chat also used the same |
@bitsikka thanks looks pretty nice, you implemented a feature and improved the codebase at the same time :) |
my pleasure! 😄 in the last force-push I tried to follow your instruction to the tee - don't know if that was necessary. I guess it allows more flexibility if needed to be used elsewhere. Thanks for all the guidance! I learned a lot! |
@bitsikka thanks for working on this! @yenda can we create a separate issue to get all those different chat headers a part of chat content and not a separate view? Here's a Figma of what I mean The intent is these are like a book cover, they should stay there on top and scroll with the rest of messages. |
cool designs! 👍 I see what you mean. |
@errorists it can be added as a first element to the message list |
@yenda cool! maybe @siphiuel can add it as part of the TtT chat PR, this header is heavily featured there? |
I take it that, what @hesterbruikman said in #7454 in the description of the issue:
the wait is over If so, and by the look of Figma design linked, wait is indeed over, and if everybody is busy, then I could work on it. I understand it will need some restructuring of UI and care is needed to touch only what is needed, and to not break anything else. |
I see what you mean |
100% of end-end tests have passed
Passed tests (45)Click to expand |
@churik 'Endless "Loading" indicator is shown if public chat appeared after sync' case is resolved for real :) in this latest push. Tested in iPhone X simulator. plus changes requested by @cammellos ready 🖐 as far as I know so far |
afk for a bit. Gonna finish review requests soon. |
Got all revisions completed in local except tests. Gonna work on the tests now before submitting updated PR tomorrow if not today. |
Did the changes requested by @cammellos. Ready for another review. |
Handled one more edge case(not unlikely for busy public chat topic) where when fresh public chat is (re)added and it is in the loading screen with loading messages from mailserver is not done. If the chat happens to receive message(s) not requested from mailserver, but from others online active in the public chat; Loading screen ought to be gone and the message(s) shown, even though request to mailserver might not have completed by then. I think there is nothing more to add or take away now. ready for review. |
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.
looks good, complicated stuff but worth the effort, well done!
Thanks @cammellos! For being my second PR, this one has been many times heftier than I would've thought. But indeed, well worth the effort over-all. @churik @hesterbruikman and @StatusSceptre more re-reviews might just be coming in (which if it is, wouldn't hurt/would help), please coordinate in what ways to proceed. |
found one more case for this PR In mobile, if you click on hash-tag of public chat in any chat; If the public chat happens to be never before added public chat - it shows blank instead of "There are no messages in this chat yet" fix coming up |
✔️ done!
|
Besides of mentioned in #7663 (comment), on last build rechecked:
Amazing job, @bitsikka ! |
93% of end-end tests have passed
Failed tests (3)Click to expand
Passed tests (43)Click to expand |
100% of end-end tests have passed
Passed tests (3)Click to expand
|
Thanks! :) |
…d issues/feature regarding chat message-views intro screens for all chats types [#7454] fix add basic copy to public chat empty screen state + chat messages-views intro screens for all chats Signed-off-by: Igor Mandrigin <i@mandrigin.ru>
Fixes #7454
Summary:
tested on iOS Simulator and Android AVD
Review notes:
Guessed the design spec - need input
status: ready