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

[MM-58285] Fix expanding call thread #774

Merged
merged 4 commits into from
Jun 13, 2024
Merged

[MM-58285] Fix expanding call thread #774

merged 4 commits into from
Jun 13, 2024

Conversation

streamer45
Copy link
Collaborator

Summary

PR fixes an issue with the call thread rendering on top of the call view, causing some buttons on the right side (e.g. leave call) to be hidden in some instances.

I had to wrestle with CSS to make sure I didn't break previously fixed things, namely:

  • Call settings menus rendering on top of the call thread in case of overlap.
  • Reactions rendering on top of calls view.
  • Call view rendering on top of the annoying onboarding button.

Please give it a try and let me now if anything unexpected broken due to this.

Ticket Link

https://mattermost.atlassian.net/browse/MM-58285

@streamer45 streamer45 added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Jun 13, 2024
@streamer45 streamer45 self-assigned this Jun 13, 2024
Copy link
Contributor

@matthewbirtch matthewbirtch 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 like it fixed it @streamer45.

I'm not sure if this is related to this PR, but if I start a call from a DM it doesn't seem to respect the active theme. This only seems to be an issue in the desktop app though. I don't see this happening on the community server.

Screenshot 2024-06-13 at 8 51 37 AM

@streamer45
Copy link
Collaborator Author

To make this work, I had to move the element in which we set the background so it may well be related. I'll take a look.

@streamer45
Copy link
Collaborator Author

@matthewbirtch It looks like this may have been broken for a while. I don't see the theme updating on the component, so it's not dynamic any longer, but it should work if you reopen the popout after changing the theme.

@matthewbirtch
Copy link
Contributor

@matthewbirtch It looks like this may have been broken for a while. I don't see the theme updating on the component, so it's not dynamic any longer, but it should work if you reopen the popout after changing the theme.

It doesn't seem to. I've tried changing my theme then starting a new call and opening up in the popout, but it doesn't seem to respect the active theme.

The issue seems to be isolated to the desktop app though and only in DMs, which is weird.

@streamer45
Copy link
Collaborator Author

streamer45 commented Jun 13, 2024

I cannot reproduce it, I tested it in a DM on Desktop, and if opened after the theme has changed, the color is updated. Maybe it's worth trying a hard refresh on both sides?

@matthewbirtch
Copy link
Contributor

I cannot reproduce it, I tested it in a DM on Desktop, and if opened after the theme has changed, the color is updated. Maybe it's worth trying a hard refresh on both sides?

I tried hard refreshes multiple times. But let me restart the server and try again

@streamer45 streamer45 removed the 1: UX Review Requires review by ux label Jun 13, 2024
@streamer45 streamer45 added this to the v0.28.0 / MM 9.10 milestone Jun 13, 2024
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Works well!

One suggestion, maybe we should put a right margin of 8px on the participants button, and a left margin of 8px on the leave button, so that when it's collapsed we'll have consistent spacing?

image

@streamer45
Copy link
Collaborator Author

CSS margins 😬 , let me review.

@cpoile
Copy link
Member

cpoile commented Jun 13, 2024

Or better yet, put a horizontal padding on the center, that way it won't potentially interfere with the complicated leave button.

@streamer45
Copy link
Collaborator Author

@cpoile I added a flex gap on the parent. Let me know if that works.

@cpoile
Copy link
Member

cpoile commented Jun 13, 2024

Yep, best way to go, works well, thanks!

@cpoile cpoile added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 13, 2024
@streamer45 streamer45 merged commit 674c56e into main Jun 13, 2024
20 checks passed
@streamer45 streamer45 deleted the MM-58285 branch June 13, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants