-
Notifications
You must be signed in to change notification settings - Fork 742
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
Add padding before first own message #5384
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.
vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/AbsMessageItem.kt
Outdated
Show resolved
Hide resolved
...rc/main/java/im/vector/app/features/home/room/detail/timeline/style/TimelineMessageLayout.kt
Outdated
Show resolved
Hide resolved
Hi there! Thanks for working on this. :) I agree with your solution. Before I approve, just want to check what's the padding (in pixels, if possible) you are using here? |
I'll do the variable rename & changelog file (.misc ?) when I've free time.
To have the same padding than the one before the bubble from other senders, I did an empty name instead of an explicit padding. That way, if you want to edit the name size, it will change that padding too. |
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 the update!
I approve the PR, but @amshakal needs to approve it to so that we can merge it.
Be aware it will increase "blank" space if you have some decoration between the last from other and first from you like day separator or read receipts. |
Nice! Since there is no measurement to compare it to, I'll request reducing the padding just a bit (around 35%) for the reasons @ganfra stated above. Happy to approve it after that :) |
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 the update. This PR will be merged if the CI is happy!
@@ -105,6 +106,9 @@ abstract class AbsMessageItem<H : AbsMessageItem.Holder> : AbsBaseMessageItem<H> | |||
} else { | |||
holder.timeView.isVisible = false | |||
} | |||
|
|||
holder.additionalTopSpace.isGone = !attributes.informationData.messageLayout.addTopMargin |
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.
Nit: it should be better to avoid using a negation here. So I would prefer to see
holder.additionalTopSpace.isVisible = attributes.informationData.messageLayout.addTopMargin
isGone
-> isVisible
and remove the !
Anyway, this is not a blocker.
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.
(waiting for sign off)
I have removed the double negation and signed-off the commit |
8c15b2e
to
17d58f2
Compare
Signed-off-by: sim <[email protected]>
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.
Perfect! Thanks!
Type of change
Content
Add a padding before the first message send by ourselves. It is the same size than a displayed name.
Motivation and context
It is easier to view our messages from others if the padding is a bit bigger.
Screenshots / GIFs
Edit: adding a table for better comparison of screnshoots
Tested devices
Checklist