-
Notifications
You must be signed in to change notification settings - Fork 1
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
Communication
: Redesign chat & alignment with iOS
#228
base: develop
Are you sure you want to change the base?
Conversation
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.
Tested the functionality on the device, looking much better now, thanks for taking care of this :D
Idk why, but the padding on the header feels weird to me, maybe its because the padding on the top is too small (and not consistent with the other paddings of the post)? But I also see that by increasing the padding there, we can fit even less posts onto the screen then.
...in/de/tum/informatics/www1/artemis/native_app/feature/metis/conversation/ui/post/PostItem.kt
Show resolved
Hide resolved
...in/de/tum/informatics/www1/artemis/native_app/feature/metis/conversation/ui/post/PostItem.kt
Show resolved
Hide resolved
...in/de/tum/informatics/www1/artemis/native_app/feature/metis/conversation/ui/post/PostItem.kt
Outdated
Show resolved
Hide resolved
...in/de/tum/informatics/www1/artemis/native_app/feature/metis/conversation/ui/post/PostUtil.kt
Outdated
Show resolved
Hide resolved
...rc/main/kotlin/de/tum/informatics/www1/artemis/native_app/core/ui/date/convertDateAndTime.kt
Outdated
Show resolved
Hide resolved
core/ui/src/main/kotlin/de/tum/informatics/www1/artemis/native_app/core/ui/date/relativeTime.kt
Show resolved
Hide resolved
I just thought of this now, but could you also verify that the changes work on Tablet and Foldable? :D |
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.
I agree with the padding adjustments Martin mentioned. While testing, I noticed an issue when replying to a message. Initially, the tag displays correctly, but after sending reply, it updates to an incorrect state and stays that way.
Screen.Recording.2024-12-22.at.15.17.46.mov
We’ve observed a similar issue with images flickering due to re-rendering, but in this case, the state remains incorrect after the render. Should we investigate this as a separate issue, or could it be related to the existing follow-up ticket on re-render problems?
Yes, that's true. I also noticed this behavior, when implementing the changes. The bug is unrelated to this PR and affects |
That's a good idea. I just tested the foldable and observed the following behavior: As you can see the PostHeader is cut in the thread UI, which is caused by the small width. What's your opinion on this @FelberMartin? I'm not quite sure how we can solve this. Is there an option to align the chat and the thread in a way that the width of both views is the same? I think this would be a better long-term solution as the thread-view is quite narrow imo. |
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.
I am not sure, but I think the padding is still off, and now it looks like the profile picture is not aligned with the AuthorRole label anymore 🤔
Regarding the issue on the foldable, I also noticed problems with this 3 column layout for another PR. Tbh I think we would be better off with just two columns when showing the ThreadUI (also on the tablet) with either
(A) Make the ThreadUI replace the ChatList (this is how it currently is on iOS / iPadOS)
(B) Hide the ConversationOverview on the left and display ChatList and ThreadUI side by side.
My take would be to do (A) for now (maybe in another PR?) and then think about what is the better strategy together with @eylulnc as soon as she deals with the navigation design as part of her thesis.
core/ui/src/main/kotlin/de/tum/informatics/www1/artemis/native_app/core/ui/date/relativeTime.kt
Show resolved
Hide resolved
I fixed it :)
I agree. I think it would be good to do it as part of another PR to avoid chaos in this PR. |
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.
Nice the alignment problem has been fixed now. However my original concern regarding the padding is still there, maybe I failed to explain it properly what my concern was.
The padding from the post card to the inner card element and between the card elements is not consistent. See the screenshot below. The horizontal (red) and vertical (green) paddings of the card and the padding between the AuthorProfilePicture and the name to the right (blue) are all different values. In iOS all these paddings are of the same size and this looks much better imo.
Co-authored-by: Martin Felber <45291671+FelberMartin@users.noreply.github.com>
Thanks for clarifying. I made the required changes and pushed them :)
Yes, this is intentional to mimic the behavior of the iOS app. |
The padding looks much better now, thank you for taking care of this :D
Hm okay, I just checked on the iPad version (because I have no iOS device), but there the posts are also displayed at the top. Can you verify this @julian-wls? Because also my intuition would be to display single posts at the top, like it is done eg in Whatsapp |
I think it looks better on iOS. This is the behavior on iPhones: |
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.
Interesting that it differs from iPhone to iPad, but then it's fine to keep it that way for Android 👍
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.
Everything looks good.
Problem Description
Currently, the chat is looking outdated and different from iOS, see #162.
Changes
This PR applies the following changes to make the chat more readable and more aligned to the iOS design:
Screenshots