-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(message-list): change item layout #9818
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(message-list): change item layout #9818
Conversation
87bbaeb to
3f8864e
Compare
rafaeltonholo
left a comment
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.
Thanks for these changes! There are a few things I believe we need to address before merging this.
legacy/ui/legacy/src/debug/kotlin/com/fsck/k9/ui/messagelist/item/MessageItemContentPreview.kt
Outdated
Show resolved
Hide resolved
legacy/ui/legacy/src/main/java/com/fsck/k9/ui/messagelist/item/MessageItemContent.kt
Show resolved
Hide resolved
legacy/ui/legacy/src/main/java/com/fsck/k9/ui/messagelist/item/MessageItemContent.kt
Show resolved
Hide resolved
|
I also want to mention a few things that are not related to these changes, but we need to address/discuss more about them:
|
|
@rafaeltonholo the implementation is not complete as I mentioned above. This just adds the available ui components and a view holder with partial integration. It doesn't make sense to continue at the moment as we need to fix the design first before further adding code. I planed to create follow up tickets, but had no time yesterday. These changes are guarded by a feature flag and I'll turn it of until we have a design update. |
I know, just no time to add tickets for. I did a hard stop yesterday and wanted to discuss the design first before adding more code. Especially as the list items already have a very large api surface, that needs to be addressed too. I think this should be covered in another issue. And the option to have alternating colors for the avatar. |
a917468 to
7dc61a8
Compare
7dc61a8 to
8c172c3
Compare
Sounds good to me! Thanks for explaining that |
rafaeltonholo
left a comment
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.
LGTM
Resolves #9654
The design is not covering the
MessageListAppearanceoptions. We need another round of discussion how to handle these.