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

Add padding before first own message #5384

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Conversation

p1gp1g
Copy link
Contributor

@p1gp1g p1gp1g commented Feb 28, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

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

Before After
Capture d’écran de 2022-03-01 00-39-42 Capture d’écran de 2022-03-01 00-35-07

Edit: adding a table for better comparison of screnshoots

Tested devices

  • Physical
  • Emulator
  • OS version(s): 9

Checklist

@bmarty bmarty requested a review from amshakal March 1, 2022 09:20
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks for the PR!
I ask for a design review from @amshakal
Also can you add a small file for the changelog? You can see instruction here

Thanks!

@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label Mar 1, 2022
@amshakal
Copy link

amshakal commented Mar 1, 2022

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?

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Mar 1, 2022

I'll do the variable rename & changelog file (.misc ?) when I've free time.

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?

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.

Copy link
Member

@bmarty bmarty left a 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.

@ganfra
Copy link
Member

ganfra commented Mar 1, 2022

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.

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Mar 1, 2022

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.

Indeed, here are some screenshots to illustrate :

  • Before, the space before our first message is the same than the space between 2 messages of the same sender. It might be preferred to save space since there is already some space between bubbles.
  • After, the space before our first message is the same than before the first from someone else. It might be preferred to have the same padding every time it is a new sender. (I think it is easier to understand) *
Before After
Capture d’écran de 2022-03-01 23-58-49 Capture d’écran de 2022-03-01 23-45-45
Capture d’écran de 2022-03-01 23-59-24 Capture d’écran de 2022-03-01 23-45-04
Capture d’écran de 2022-03-02 00-04-08 image

* Comparison between same padding with reaction before/after
before :

2 from the same sender Our first message
Capture d’écran de 2022-03-02 00-21-06 image

after :

Other's first message Our first message
image image

@amshakal
Copy link

amshakal commented Mar 2, 2022

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 :)

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Mar 2, 2022

It is indeed better with a smaller padding. I've a bit edited how it works to use a 12dp space instead of the empty name.

Capture d’écran de 2022-03-02 17-53-08
Capture d’écran de 2022-03-02 17-53-33

Copy link
Member

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

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.

@bmarty
Copy link
Member

bmarty commented Mar 8, 2022

Ah, I will need a sign off to be able to merge this PR.

@p1gp1g can you add a comment to this PR with your sign off please?

Copy link
Member

@bmarty bmarty left a 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)

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Mar 8, 2022

I have removed the double negation and signed-off the commit

@p1gp1g p1gp1g force-pushed the bubble-space branch 2 times, most recently from 8c15b2e to 17d58f2 Compare March 8, 2022 16:26
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thanks!

@bmarty bmarty enabled auto-merge March 10, 2022 12:34
@bmarty bmarty merged commit 551b827 into element-hq:develop Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants