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.

Sign up for GitHub

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

Jump to bottom

Fix display name and date alignment #14304

Merged
merged 15 commits into from
Jan 30, 2023
Merged

Fix display name and date alignment #14304

Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
53bcc26
align baseline
luacmartins Jan 13, 2023
f79aa03
use correct height and padding for date
luacmartins Jan 13, 2023
1ce1fdf
Merge branch 'main' into cmartins-alignBaseline
luacmartins Jan 19, 2023
e55a447
rm height
luacmartins Jan 19, 2023
fe68b02
add lineheight
luacmartins Jan 20, 2023
95f0fe2
Merge branch 'main' into cmartins-alignBaseline
luacmartins Jan 23, 2023
a2246fe
update styles
luacmartins Jan 23, 2023
c2620ae
rm negative margin from messages
luacmartins Jan 23, 2023
fd35c68
add padding to single images
luacmartins Jan 23, 2023
50e1ed5
fix console error
luacmartins Jan 23, 2023
0d6b102
use margin
luacmartins Jan 23, 2023
1746629
get isAttachment from server
luacmartins Jan 23, 2023
75842b7
move logic to conditional
luacmartins Jan 23, 2023
276d598
Merge branch 'main' into cmartins-alignBaseline
luacmartins Jan 27, 2023
ac94ca3
adjust margins
luacmartins Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1328,8 +1328,9 @@ const styles = {
flexShrink: 0,
color: themeColors.textSupporting,
fontSize: variables.fontSizeSmall,
height: 24,
height: 20,
lineHeight: variables.lineHeightXLarge,
paddingBottom: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to get rid of the padding bottom. However, for image attachments, if the image attachment is the first thing in the message group (it comes right after the sender name) we should give it some top margin of say 4-8px.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just read the Slack thread linked to the issue and it seems like we also want to do some cleanup on the negative margins used for the message text, as well as add some padding for images. I'll add that to this PR.

That being said, I'm having issues aligning the sender name & date at the baseline. alignItems: 'baseline' is supported in RN and works well on browsers, but the Pressable component wrapping the sender name text is throwing off the alignment on iOS/android native since it's baseline is different from the text baseline. Here's how they behave:

iOS
ios

android
android

I've spent some time trying to work around this issue, but no luck so far. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Take a look at the discussion from this one: #14148

We actually learned that baseline doesn't always work on Android so we might want to avoid that.

There was a proposal that seemed to fix the alignment by always making sure the big text (sender) and small text (timestamp) had the same lineheight and height. So maybe we could try that first?

Copy link
Contributor

Choose a reason for hiding this comment

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

So after seeing this, it does feel like the sender is too close to the message, but maybe that's because the negative margin is still there. So let me know once you start playing around with that stuff too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a proposal that seemed to fix the alignment by always making sure the big text (sender) and small text (timestamp) had the same lineheight and height. So maybe we could try that first?

I tried this and the other solutions in that issue but none of them seem to work. Maybe I'm holding something wrong?

I'd appreciate any other suggestions as I'm struggling to find a solution that doesn't add any padding/margins to the text.

Copy link
Contributor Author

@luacmartins luacmartins Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@shawnborton I updated the branch with the following:

  1. Added lineHeight: 20px to the senders name and removed the bottom padding
  2. Added lineHeight: 16px and paddingTop: 4px to the timestamp

Now the header + single line message sums up to 40px and both seem to be aligned at the baseline:
Screenshot 2023-01-23 at 4 19 09 PM

Screenshot 2023-01-23 at 4 19 02 PM

In addition, I removed the negative margin from chatItemMessage and added a conditional 8px top margin to attachments when displayed with the header:

Screenshot 2023-01-23 at 4 27 41 PM

Let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aneequeahmad let's see what @shawnborton says about my changes above!

Copy link
Contributor

Choose a reason for hiding this comment

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

That feels pretty great to me! Thanks for pushing through this one and getting to a good solution!

Copy link
Contributor

Choose a reason for hiding this comment

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

@luacmartins Code changes looks good to me too. That's a pretty neat solution. @shawnborton Thank-you for your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both for the help!

},

chatItemMessage: {
Expand Down