Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fixes reaction row rendering out of view on message bubbles #8542

Closed
wants to merge 1 commit into from

Conversation

yaya-usman
Copy link
Contributor

@yaya-usman yaya-usman commented May 9, 2022

Fixes: vector-im/element-web#22093

Issue reported by @frakic

Signed-off-by: Yaya Usman [email protected]

Before:
Capture2

After:
Capture1

Type: defect


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fixes reaction row rendering out of view on message bubbles (#8542). Contributed by @yaya-usman.

@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label May 9, 2022
@yaya-usman yaya-usman marked this pull request as ready for review May 9, 2022 14:27
@yaya-usman yaya-usman requested a review from a team as a code owner May 9, 2022 14:27
@SimonBrandner
Copy link
Contributor

I am confused - does this make the emojis go in two lines with the second one being indented from the right?

@yaya-usman
Copy link
Contributor Author

I am confused - does this make the emojis go in two lines with the second one being indented from the right?

Apparently yes, when the size of the screen is shrinked to a smaller width, it just wraps the emojis or confine the emojis inside its container instead of going off screen. I think the original issue explains the problem better.

@SimonBrandner
Copy link
Contributor

SimonBrandner commented May 9, 2022

I am confused - does this make the emojis go in two lines with the second one being indented from the right?

Apparently yes, when the size of the screen is shrinked to a smaller width, it just wraps the emojis or confine the emojis inside its container instead of going off screen. I think the original issue explains the problem better.

I think we would want a full fix for the issue before merging - avoid the wrapping and avoid them going off-screen - they should behave the same way they do on the Modern layout. Do you think you could try to take a look into that?

@yaya-usman
Copy link
Contributor Author

@SimonBrandner check this out, i just updated the code and it seems to work fine like the modern layout maybe this is what you're implying, i want to confirm before pushing my changes.

Modern chat layout
wrapperModern

Bubble chat layout
reactionBubble

@SimonBrandner
Copy link
Contributor

This is much better! One more thing - do you think it would be possible to show all the reactions on one line and if they were to overflow, show the Show all button?

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Clearing team review for now - please re-request review when ready

@yaya-usman
Copy link
Contributor Author

yaya-usman commented May 10, 2022

This is much better! One more thing - do you think it would be possible to show all the reactions on one line and if they were to overflow, show the Show all button?

To be honest i don't know if it's possible but this is the default behavior of element web, it includes show all if the emojis is getting many, and i think it's more logical that way, it's not like it includes show all at the beginning, it does only when the emojis starts to become many. so i don't really find any reason to list out all the emojis and try to include show all when it overflows. Also if we decide to do it as you have stated, the emojis will get longer than the bubble width, i mean it will longer than the chat bubble in width, which will make it look awkward. This is what i think

@yaya-usman
Copy link
Contributor Author

Capture1

@SimonBrandner
Copy link
Contributor

image

Sorry, I don't think I was very clear...

What I was suggesting was a behaviour very similar to the above. But we wouldn't wrap - once the emoji list would get too long we would show the Show all button instead of wrapping.

Would that be possible?

@yaya-usman
Copy link
Contributor Author

I think i understand you now, Let me try and see if i can come up with anything based on my understanding if not, i might ask for more clearance from you, thanks

@MadLittleMods MadLittleMods added the Z-Community-PR Issue is solved by a community member's PR label Jun 1, 2022
@luixxiul
Copy link
Contributor

Since the issue has been fixed, I think this PR can be closed.

@SimonBrandner
Copy link
Contributor

Since the issue has been fixed, I think this PR can be closed.

Closing as per ^, thanks for tagging!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems 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.

Reactions go off screen when using message bubbles
4 participants