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

chat: fb-like msg style re-design #107

Merged
merged 21 commits into from
Aug 1, 2020
Merged

chat: fb-like msg style re-design #107

merged 21 commits into from
Aug 1, 2020

Conversation

erelado
Copy link
Collaborator

@erelado erelado commented Jan 23, 2020

Change it to look much more fb-like.

From:
image
to:
image

@erelado erelado requested a review from vednoc January 23, 2020 14:20
Copy link
Owner

@vednoc vednoc left a comment

Choose a reason for hiding this comment

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

Spent a little while testing it and there are small bugs. I would rather not mess around with message padding/margin because it varies from language to language AFAIK. The removal of invisble-space is also not ideal.

The following works without issues, plus it looks great! The second rule is for bubble's bottom padding - it has 6px padding on top and 8px padding on the bottom for some reason. The default font stack on Linux looks out of place, so I'd like to know what it looks like on Windows.

-                ._1RNhZ { margin: -16px -54px -5px 4px !important }
-                .-N6Gq { padding: 6px 64px 8px 9px !important }
-                .selectable-text.invisible-space:after { display: none }
+                ._1RNhZ { margin-top: -16px i }
+                .-N6Gq { padding-bottom: 6px i }

@erelado
Copy link
Collaborator Author

erelado commented Jan 25, 2020

[...] so I'd like to know what it looks like on Windows.

It looks fine:
image

I really do think though finding a better solution to fit the timestamp next to text will benefit this message-style a lot.

@vednoc
Copy link
Owner

vednoc commented Jan 25, 2020

Here is what it looks like on my end:

image

With your changes:

image

Additional tweaks will be needed to make it work with LTR/RTL layouts.

@erelado
Copy link
Collaborator Author

erelado commented Jan 25, 2020

Interesting. In both cases on your end, it seems like the timestamp is a bit beneath the text itself.
As about the timestamp on my end: maybe that's a Right-to-Left issue?

@vednoc
Copy link
Owner

vednoc commented Jan 25, 2020

We can adjust the timestamp. The problem is in adjusting both LTR and RTL layouts in all cases, which is not really an ideal solution IMO.

vednoc added a commit that referenced this pull request Apr 30, 2020
Fixes inconsistencies in RTL layouts.

The fix is only applied to RTL messages so that LTR messages are left
as-is, and there is no need to disable this setting.

If there are issues, please let us know.

Relates to #107.
@erelado
Copy link
Collaborator Author

erelado commented Jun 13, 2020

@erelado erelado closed this Jun 13, 2020
@vednoc
Copy link
Owner

vednoc commented Jun 13, 2020

@E-RELevant I suppose we could (re)use this PR to port this feature to v3. What do you think?

Strangely enough, the `{ md }, > div > div` will actually compile
successfully, yet produce no output, and it took me a _while_ to figure
that out.

A workaround is to put interpolation at the very end, and for whatever
reason it _just works_. However, it seems like it doesn't play well with
the `&:not({ ... })` selector so that one had to be split to two lines.
@vednoc
Copy link
Owner

vednoc commented Jul 28, 2020

@E-RELevant I only need to port RTL hacks for fb-like tails to v3 and this PR will be ready for merge. It will need a lot more testing once that's implemented, but let me know if you find bugs in the base styles.

@vednoc vednoc merged commit 7223bf8 into develop Aug 1, 2020
@vednoc vednoc deleted the develop-fb-msg branch August 18, 2020 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants